Opened 8 years ago

Closed 4 years ago

Last modified 3 years ago

#3523 closed enhancement (implemented)

Allow controllers to post HS descriptors to the HSDir system

Reported by: rransom Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: tor-hs, controller, nickm-review, 027-triaged-1-in, SponsorS
Cc: Actual Points:
Parent ID: #8993 Points: small-remaning
Reviewer: Sponsor: SponsorR

Description


Child Tickets

Change History (32)

comment:1 Changed 8 years ago by 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?
  • 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?

comment:2 in reply to:  1 Changed 8 years ago by rransom

Priority: normalminor

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.

  • 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 much easier.

comment:3 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: unspecified

comment:4 Changed 6 years ago by nickm

Keywords: maybe-proposal added

comment:5 Changed 6 years ago by nickm

Keywords: tor-hs added

comment:6 Changed 6 years ago by nickm

Component: Tor Hidden ServicesTor

comment:7 Changed 6 years ago by arma

Parent ID: #8993

comment:8 Changed 4 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.7.x-final

Worth looking at during 0.2.7 triage IMO

comment:9 Changed 4 years ago by donncha

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

comment:10 Changed 4 years ago by nickm

Owner: rransom deleted
Status: newassigned

comment:11 Changed 4 years ago by donncha

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

comment:12 Changed 4 years ago by donncha

Status: assignedneeds_review

comment:13 Changed 4 years ago by yawning

Status: needs_reviewneeds_revision

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.

comment:14 Changed 4 years ago by donncha

Status: needs_revisionneeds_review

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

comment:15 in reply to:  14 ; Changed 4 years ago by yawning

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.

Last edited 4 years ago by yawning (previous) (diff)

comment:16 Changed 4 years ago by yawning

Status: needs_reviewneeds_revision

comment:17 in reply to:  15 ; Changed 4 years ago by dgoulet

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, 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:
    Descriptor = The text of the descriptor formatted as specified
    in rend-spec.txt section 1.3 or empty string on failure.
    

comment:18 in reply to:  17 Changed 4 years ago by yawning

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

comment:20 Changed 4 years ago by dgoulet

Keywords: controller added; maybe-proposal removed
Status: needs_revisionneeds_review

comment:21 Changed 4 years ago by donncha

comment:22 in reply to:  21 Changed 4 years ago by dgoulet

Status: needs_reviewneeds_revision

Replying to donncha:

Updated the code to remove some redundant code
https://github.com/DonnchaC/tor/compare/ca03b10b0c39a02fd66824d04deccf40cbd66951...3234-patch

In handle_control_hspost()

  • I think here it should be 512, see control-spec.txt (512 Syntax error in command argument)
    connection_printf_to_buf(conn, "552 Unexpected argument \"%s\"\r\n",
                             arg);
    
  • This is not necessary. No chance that descs contains more than one descriptor so you should keep desc intact (not setting it to NULL) and free it in the done label (even if NULL). If anything pass the desc allocation in the future makes a goto done, you'll be safe already :).
SMARTLIST_FOREACH(descs, rend_encoded_v2_service_descriptor_t *, d, {
...
  • intro_content can leak. Fun fact, rend_parse_v2_service_descriptor() can set intro_content and still go on error after... Now, tbh I would fix rend_parse_v2_service_descriptor() instead here to ONLY sets out pointers when it actually succeed ;). But better safe than sorry, set it to NULL and free it once you are done. (If you feel like it, do a commit on top to fix rend_parse.., that would be neat :)

In directory_post_to_hs_dir

  • This will *not* trigger an UPLOAD event which I think it should according to the spec?
+    } else {
+      /* Determine responsible dirs. */
+      if (hid_serv_get_responsible_directories(.......

Rest looks good! We are getting there! :D

comment:23 Changed 4 years ago by donncha

Thanks for the review again dgoulet. Those changes have pushed! https://github.com/DonnchaC/tor/compare/ca03b10b0c39a02fd66824d04deccf40cbd66951...3234-patch

comment:24 Changed 4 years ago by donncha

Status: needs_revisionneeds_review

comment:25 Changed 4 years ago by yawning

Keywords: nickm-review added

Tagging for nickm-review, since both dgoulet and I have looked at this.

comment:26 Changed 4 years ago by nickm

Notes:

  • Somebody has to write a changes file here.
  • It would be pretty cool if the command parsing part of hspost handler were split into a separate function.
  • Tests would be great, for as much of this as possible.
  • Shouldn't we be checking the return value of read_escaped_data?
  • Doesn't the documentation on directory_post_to_hs_dir need to change to match its new arguments?
  • Make sure that 'make check-spaces' passes.


comment:27 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking some tickets as triaged-in for 0.2.7 based on early triage

comment:28 Changed 4 years ago by isabela

Keywords: SponsorS SponsorR added
Points: small-remaning
Priority: minornormal
Version: Tor: 0.2.7

comment:29 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

comment:30 in reply to:  26 Changed 4 years ago by yawning

Status: needs_revisionneeds_review

Because it would be a big shame if this didn't make it into 0.2.7.1-alpha...

Branch: https://github.com/Yawning/tor/compare/feature3523_027

Changes:

  • Rebase/squash/and rewriting the commit log from comment 23.

fd11b505ea4fce0a493bcd587b7697c4463b7538

  • Make sure that 'make check-spaces' passes.

308373e29a52311ccf2e6dd034b105b7ee6eeafa

  • Doesn't the documentation on directory_post_to_hs_dir need to change to match its new arguments?

0b30b698a3449c4ca78a83e8f570868b5095e828

  • Somebody has to write a changes file here.

b075684f85a3a6a5f721d793f89a99b2873dd8c9

  • Fixed directory_post_to_hs_dir to not double-free hs_dirs.

b726a7574534f03e69947c6a3e604df580aef746

Other things nickm brought up that were not done, and why:
Replying to nickm:

  • It would be pretty cool if the command parsing part of hspost handler were split into a separate function.

Hmm, I don't think HSPOST is anywhere near as bad as HSFETCH, so I'm not going to do this, using the same excuse dgoulet did for HSFETCH.

  • Tests would be great, for as much of this as possible.

See above. Someone with a better understanding of the HS code and what has mock implementations available could do this.

  • Shouldn't we be checking the return value of read_escaped_data?

We could check if it returns 0 I guess? rend_parse_v2_service_descriptor will do the right thing in that case though...

Tentatively "needs-review"-ing this again. The events on descriptor upload would be *really nice* to have since it will allow people using the new ADD_ONION stuff to know when their HS is ready.

comment:31 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

I can't see anything broken here. Merging!

comment:32 Changed 3 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR
Note: See TracTickets for help on using tickets.