Allow controllers to post HS descriptors to the HSDir system

changed milestone to %Tor: 0.2.7.x-final

added 027-triaged-1-in SponsorS component::core tor/tor controller milestone::Tor: 0.2.7.x-final nickm-review parent::8993 points::small-remaning priority::medium resolution::implemented sponsor::R status::closed tor-hs type::enhancement version::tor 0.2.7 labels

Do you mean:

• let a controller tell Tor to upload a descriptor to one or more HSDirs?
• let a controller tell Tor to upload a descriptor to the appropriate HSDirs?
• let a controller tell a Tor that is acting as an HSDir to store and serve a descriptor?
• let a controller tell Tor to store a descriptor as though it had been received from an HSDir?
• all/some/none of the above?

And what's the use case you have in mind?

Replying to nickm:

Do you mean:

• let a controller tell Tor to upload a descriptor to one or more HSDirs?
• let a controller tell Tor to upload a descriptor to the appropriate HSDirs?

This one.

• let a controller tell a Tor that is acting as an HSDir to store and serve a descriptor?
• let a controller tell Tor to store a descriptor as though it had been received from an HSDir?

This is ticket #3522 (moved).

• all/some/none of the above?

And what's the use case you have in mind?

This would make the existing ‘PublishHidServDescriptors 0’ configuration option less useless, as well as making testing an implementation of some possible designs for #3507 (moved) much easier.

Trac:
Priority: normal to minor

Trac:
Milestone: Tor: 0.2.3.x-final to Tor: unspecified

Trac:
Keywords: N/A deleted, maybe-proposal added

Trac:
Keywords: maybe-proposal deleted, maybe-proposal tor-hs added

Trac:
Component: Tor Hidden Services to Tor

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

Worth looking at during 0.2.7 triage IMO

Trac:
Milestone: Tor: unspecified to Tor: 0.2.7.x-final

I currently have two use cases for this feature.

My malicious HSDir detection tool needs to be able to upload custom HS descriptors to a specific HSDir as defined by the HSDir's fingerprint. In the other use case, to create a hidden service load balancer [1] I would need to be able to upload a descriptor to its 3 responsible HSDir's.

For now it would be great to just be able to specific HSDir's, determining the responsible directorys can be offloaded to the client code without much difficulty.

[1] !https://lists.torproject.org/pipermail/tor-talk/2015-March/037238.html

Trac:
Status: new to assigned
Owner: rransom to N/A

I've had a go at writing a patch for this. Thanks asn and dgoulet for the feedback already! Review and any suggestions welcome. Thanks in advance! https://github.com/DonnchaC/tor/compare/ca03b10b0c39a02fd66824d04deccf40cbd66951...3523

Trac:
Status: assigned to needs_review

Casual review:

• (General) This needs a control-spec.txt patch documenting the behavior.
• control.c:handle_control_hspost()
  • This routine leaks memory like a sieve.
  • cp = (char*) body; don't cast off the const.
• rendservice.c:directory_post_to_hs_dir()
  • This needs documentation that it will obliterate the contents of hs_dirs and free the pointer.
  • Leaks a smartlist_t when hs_dirs != NULL.

There may be more things wrong, but this should be a start.

Trac:
Status: needs_review to needs_revision

Thanks for the review. I'm new to C, so apologies for all the rookie mistakes! I've attempted to fix the memory leaks that I can see. I've updated the code repo and also pushed a patch for the tor-spec.

"cp = (char*) body; don't cast off the const". This didn't seem right to me either. I was following the style of handle_control_postdescriptor(), where a '\0' is placed at the end of the argument list to stop argument parsing. As such a need a non const char pointer. What do you think I should do instead? Copy the argument line into a new char array and the parse the arguments from that instead?

Thanks again for the review

https://github.com/DonnchaC/tor/compare/ca03b10b0c39a02fd66824d04deccf40cbd66951...3523 https://github.com/DonnchaC/torspec/compare/bc348154c63f46fc6cc56c49d339fcd0aab78f3d...3523

Trac:
Status: needs_revision to needs_review

Replying to donncha:

Thanks for the review. I'm new to C, so apologies for all the rookie mistakes! I've attempted to fix the memory leaks that I can see. I've updated the code repo and also pushed a patch for the tor-spec.

No worries, thanks for the code.

"cp = (char*) body; don't cast off the const". This didn't seem right to me either. I was following the style of handle_control_postdescriptor(), where a '\0' is placed at the end of the argument list to stop argument parsing. As such a need a non const char pointer. What do you think I should do instead? Copy the argument line into a new char array and the parse the arguments from that instead?

Thanks again for the review

https://github.com/DonnchaC/tor/compare/ca03b10b0c39a02fd66824d04deccf40cbd66951...3523

Hmm. Perhaps something like https://gist.github.com/Yawning/b41f29b6893dcdb83cc1 may resolve the remaining issues (NB: Untested because I don't have a HS descriptor etc.).

Make sure I didn't mess anything up, and probably start seeking a 2nd reviewer (dgoulet maybe since he also has been doing control port stuff recently).

https://github.com/DonnchaC/torspec/compare/bc348154c63f46fc6cc56c49d339fcd0aab78f3d...3523

This looks ok to me.

Trac:
Status: needs_review to needs_revision

Hmm. Perhaps something like https://gist.github.com/Yawning/b41f29b6893dcdb83cc1 may resolve the remaining issues (NB: Untested because I don't have a HS descriptor etc.).

Make sure I didn't mess anything up, and probably start seeking a 2nd reviewer (dgoulet maybe since he also has been doing control port stuff recently).

Here is the actual gist ;) --> https://gist.github.com/Yawning/3feb8c26c3be3355f0e3

I was reviewing but then you fixed the patch with the above so my review is a bit useless now ;). As for the memchr() and *cp++ = '\n' thingy, that's just plain bad. Assigning something to an originally const char pointer is not advise. Actually, if that memory was not in a writable section, you hit a big segfault. POSTDESCRIPTOR code should be fixed because this is very very bad practice.

The rest of the patch is fine. I'll have another look at the next revision :).

Here is my review of the tor-spec:

• This will be problematic because parser and library expect HSAddress and AuthType even if none can be provided. This is why AuthType has a "NOAUTH" value and HSAddress, with #14847 (moved), will have a "UNKNOWN" value. Thus, I would suggest not putting them optional.

- "650" SP "HS_DESC" SP Action SP HSAddress SP AuthType SP HsDir [SP DescriptorID]
- [SP "REASON=" Reason]
+ "650" SP "HS_DESC" SP Action [SP HSAddress] [SP AuthType] SP HsDir
+ [SP DescriptorID] [SP "REASON=" Reason]

• Server = Fingerprint, you should allow LongName instead. node_get_by_hex_id() in the code supports that format.
• "are provide" --> "are provided" ?
• Descriptor should be defined. Again, like in #14847 (moved):

Descriptor = The text of the descriptor formatted as specified
in rend-spec.txt section 1.3 or empty string on failure.

Replying to dgoulet:

I was reviewing but then you fixed the patch with the above so my review is a bit useless now ;). As for the memchr() and *cp++ = '\n' thingy, that's just plain bad. Assigning something to an originally const char pointer is not advise. Actually, if that memory was not in a writable section, you hit a big segfault. POSTDESCRIPTOR code should be fixed because this is very very bad practice.

Yeah probably. Just tor_strdup() body or something, and make sure to free the copy on exit.

The rest of the patch is fine. I'll have another look at the next revision :).

Actually I screwed up.

The line that reads tor_free(desc); should be rend_encoded_v2_service_descriptor_free(desc);, since desc->desc_str needs to be cleaned up. The routine does the right thing on NULL, so a straight forward replacement is fine. A dumb mistake that I shouldn't make. :(