Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#23056 closed defect (fixed)

prop224: Intro point aren't transfered between services on HUP

Reported by: dgoulet Owned by: dgoulet
Priority: High Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, tor-hs
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

For the current prop224 upstream code, the move_intro_points() function doesn't work as expected, actually it's very broken.

First of all, it is impossible to move intro points with the current condition because the newly created service (dst) doesn't have any descriptor. Thus, this if() is basically dead code and we never move intro points.

if (src->desc_current && dst->desc_current) {
  move_descriptor_intro_points(src->desc_current, dst->desc_current);
...

The fix is to move the *descriptor(s)* and not only the intro points because the service needs the descriptor signing key that cross certify every IP authentication key. So, we really need to move the full thing from one service to the other else we would have to re-sign everything.

Child Tickets

Change History (8)

comment:1 Changed 2 years ago by dgoulet

Status: newneeds_review

Fix is not very complicated.

See branch: ticket23056_032_01

comment:2 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

Should the "move" function check whether "dst" already has descriptors, and either free them, or give a warning?

Let's also add a some description to the function documentation about _why_ we would want to do this.

comment:3 Changed 2 years ago by asn

Priority: MediumHigh

comment:4 Changed 2 years ago by asn

Notes: Seems like David's patch fixes the intro point issue. But there is also a rev counter issue where after a HUP we start overwriting the rev counter:

Aug 25 16:16:10.826 [warn] Found rev counter for rX5ANVTQcBhcuJbbF3EQ/TFq0Cnx4U+2JC/h736/szM: 18
Aug 25 16:16:10.826 [warn] [!] Adding rev counter 19 for rX5ANVTQcBhcuJbbF3EQ/TFq0Cnx4U+2JC/h736/szM!
Aug 25 16:16:10.826 [warn] [!] Adding rev counter 18 for rX5ANVTQcBhcuJbbF3EQ/TFq0Cnx4U+2JC/h736/szM!

Here we go from rev counter 18 to 19, then back to 18...

comment:5 Changed 2 years ago by asn

Status: needs_revisionneeds_review

OK. Pushed new branch in ticket23056_v2 in my repo.

It addresses Nick's comments, and also the bug from comment:4 (the problem was that we were not copying the HS state during SIGHUP). It's also rebased on latest master.

Let me know if you have any questions!

comment:6 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:7 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged. But: Should there be tests here? And has one of you tried this? (having a service and HUPping it?)

comment:8 Changed 2 years ago by dgoulet

Yes we actually found the issue while testing the HUPing. It does work now for sure, I can HUP my long time running service and it is still reachable and working correctly with the descriptors.

Unit tests hopefully will be part of #23310

Note: See TracTickets for help on using tickets.