Simplify hid_serv_responsible_for_desc_id

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

changed milestone to %Tor: 0.2.2.x-final

added component::core tor/tor milestone::Tor: 0.2.2.x-final owner::rransom parent::2552 priority::high resolution::fixed status::closed tor-hs type::defect labels

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.

Trac:
Status: new to assigned
Summary: Audit hid_serv_responsible_for_desc_id to Simplify hid_serv_responsible_for_desc_id
Priority: critical to major

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

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

Trac:
Status: assigned to needs_review

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

Trac:
Milestone: Tor: 0.2.1.x-final to Tor: 0.2.2.x-final

Oops. I need to fix that patch.

Trac:
Status: needs_review to assigned

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

Trac:
Status: assigned to needs_review

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.

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.

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.

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 ?

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.

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

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

Good point; fixing that too and merging.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Trac:
Parent: N/A to #2552 (moved)

Trac:
Keywords: N/A deleted, tor-hs added

Trac:
Component: Tor Hidden Services to Tor

closed

moved to tpo/core/tor#2732 (closed)