Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#2732 closed defect (fixed)

Simplify hid_serv_responsible_for_desc_id

Reported by: rransom Owned by: rransom
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs
Cc: Actual Points:
Parent ID: #2552 Points:
Reviewer: Sponsor:

Description

hid_serv_responsible_for_desc_id looks more complex than I think it should be. I suspect it's also wrong.

Child Tickets

Change History (17)

comment:1 Changed 9 years ago by rransom

Priority: criticalmajor
Status: newassigned
Summary: Audit hid_serv_responsible_for_desc_idSimplify hid_serv_responsible_for_desc_id

Currently, hid_serv_responsible_for_desc_id:

  1. calls hid_serv_acting_as_directory to check that the relay is configured as a hidden service directory, and that the relay is listed in the consensus with the HSDir flag (and returns 0 otherwise);
  2. calls hid_serv_get_responsible_directories to get a list of up to REND_NUMBER_OF_CONSECUTIVE_REPLICAS (currently 3) HSDir nodes whose ‘identity digest’s follow the hidden service descriptor's ‘descriptor ID’;
  3. uses rend_id_is_in_interval to verify that the relay's ‘identity digest’ is in the circular interval after the HS ‘descriptor ID’ and before or equal to the ‘identity digest’ of the last relay in the list returned by hid_serv_get_responsible_directories (and returns 0 otherwise). (Note that this relies on the undocumented fact that hid_serv_get_responsible_directories adds relays to the list in the order in which it encounters them.)

This is equivalent to the following (much simpler) behaviour:

  1. Call hid_serv_get_responsible_directories to get a list of all relays responsible for the ‘descriptor ID’, then
  2. return 1 if this relay is in the resulting list, or 0 if it is not.

comment:2 Changed 9 years ago by rransom

In other words, hid_serv_get_responsible_directories is correct, but unnecessarily complex.

comment:3 Changed 9 years ago by rransom

Status: assignedneeds_review

See task2732 ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git task2732 ) for a patch to simplify this function.

comment:4 Changed 9 years ago by rransom

Milestone: Tor: 0.2.1.x-finalTor: 0.2.2.x-final

This probably belongs on 0.2.2.x -- the current behaviour does produce the correct answer.

comment:5 Changed 9 years ago by rransom

Status: needs_reviewassigned

Oops. I need to fix that patch.

comment:6 Changed 9 years ago by rransom

Status: assignedneeds_review

See task2732-v2 ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git task2732-v2 ) for a corrected patch.

comment:7 Changed 9 years ago by nickm

I'm not sure this is the behavior we want; I think the old thing might be deliberate but broken.

I think that the intention might have been for us to store the descriptor not only if we are listed as an HSDir at the right place in the consensus, but also if we are not an HSDir but we *are* listed in the right place in the consensus. This is broken since the check for hid_serv_acting_as_directory() depends on the HSDir flag.

So I'd like to make sure that the intent *isn't* for non-HSDir routers to store descriptors anyway before we merge this.

comment:8 in reply to:  7 Changed 9 years ago by rransom

Replying to nickm:

I'm not sure this is the behavior we want; I think the old thing might be deliberate but broken.

I think that the intention might have been for us to store the descriptor not only if we are listed as an HSDir at the right place in the consensus, but also if we are not an HSDir but we *are* listed in the right place in the consensus. This is broken since the check for hid_serv_acting_as_directory() depends on the HSDir flag.

hid_serv_acting_as_directory has always checked whether the relay is marked with the HSDir flag in the consensus, and hid_serv_responsible_for_desc_id has always called hid_serv_acting_as_directory.

However, before commit bf2717ff, hid_serv_responsible_for_desc_id determined whether a relay is responsible for a given descriptor ID in a different way. The prior method would have broken if less than REND_NUMBER_OF_CONSECUTIVE_REPLICAS relays had the HSDir flag (although those versions would have not attempted to look up HS descriptors at all in that case), but seems to be equivalent in other cases.

So I'd like to make sure that the intent *isn't* for non-HSDir routers to store descriptors anyway before we merge this.

It looks like the intent was for non-HSDir routers to not store descriptors, but that's probably a bad design.

comment:9 Changed 9 years ago by nickm


20:02 < nickm> rransom: I'm hoping to understand what you mean by the last 
               sentence of your latest comment on #2732
20:02 < nickm> what's probably a bad design, and why?
20:03 < nickm> or maybe I should just comment there
20:03 < rransom> Only accepting HS descriptors if a relay is listed as an HSDir 
                 is a bad design,
20:04 < rransom> because the relay *will not* use the same consensus to make 
                 that decision as clients will use to decide whether to post to 
                 or fetch from a relay.
20:04 < nickm> ok.  so instead should we forget about 2732 and instead fix that 
               issue?
20:04 < rransom> Probably.
20:04 < rransom> Yes.
20:05 < nickm>  okay if I copy-paste this onto #2732?
20:05 < rransom> wwYes.

comment:10 Changed 8 years ago by nickm

So based on all that discussion, what is the *right* behavior here? Should we just always accept a hidden service descriptor regardless, or should we add some sloppiness to the existing range test... or should we just add some comments here and remove the is_hs_dir test from hid_serv_acting_as_directory ?

comment:11 in reply to:  10 Changed 8 years ago by rransom

Replying to nickm:

So based on all that discussion, what is the *right* behavior here? Should we just always accept a hidden service descriptor regardless, or should we add some sloppiness to the existing range test... or should we just add some comments here and remove the is_hs_dir test from hid_serv_acting_as_directory ?

We should remove the is_hs_dir test from hid_serv_acting_as_directory (along with the related do-we-have-a-consensus test) for now. Adding some sloppiness to the range test in hid_serv_responsible_for_desc_id would be a Good Thing, too, but that would be tricky to get right, and stabilizing the set of HSDir nodes is still necessary and would make loosening the range test less important.

I don't like the idea of having all relays accept all HS descriptors -- that would make filling relays' RAM easier without a significant benefit.

comment:12 Changed 8 years ago by nickm

Okay; see branch bug2732-simpler in my public repository.

comment:13 Changed 8 years ago by rransom

bug2732-simpler should work, but you could also rip out the code in that function which retrieves the current consensus.

comment:14 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Good point; fixing that too and merging.

comment:15 Changed 8 years ago by arma

Parent ID: #2552

comment:16 Changed 7 years ago by nickm

Keywords: tor-hs added

comment:17 Changed 7 years ago by nickm

Component: Tor Hidden ServicesTor
Note: See TracTickets for help on using tickets.