Opened 12 months ago

Last modified 3 weeks ago

#25417 merge_ready defect

HSFETCH support for v3 Hidden Services

Reported by: atagar Owned by: neel
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-control, tor-hs, prop224-extra, onionbalance, 041-proposed
Cc: dmr, s7r Actual Points:
Parent ID: #28841 Points: 0.5
Reviewer: dgoulet Sponsor:

Description

Hi David, just realized that we probably don't have a tracking ticket for our lack of v3 HSFETCH support. Your recent spec update made reference to #20699 but that's no longer open so filing another one for this.

Child Tickets

TicketStatusOwnerSummaryComponent
#25920closedHSFETCH for v3 servicesCore Tor/Tor

Change History (27)

comment:1 Changed 10 months ago by dgoulet

Keywords: tor-control tor-hs added
Milestone: Tor: unspecified

comment:2 Changed 10 months ago by cypherpunks

Parent ID: #25955

comment:3 Changed 10 months ago by dgoulet

Parent ID: #25955

Unparenting, this has nothing to do with v2 deprecation roadmap. We might need it or not in the future. But if we do, lets provide a rationale instead of flat parenting. (And if it is about OnionBalance v3 support, please make sure with the maintainer that it is in fact needed.)

comment:4 Changed 10 months ago by dmr

Cc: dmr added
Keywords: prop224-extra added

Tagging with prop224-extra in the interim until we know for sure whether its needed. See tor-dev email thread, notably this.

comment:5 Changed 6 months ago by neel

Cc: neel@… added
Owner: set to neel
Status: newassigned

comment:6 Changed 6 months ago by neel

Cc: neel@… removed
Owner: neel deleted

comment:7 Changed 6 months ago by neel

Status: assignednew

comment:8 Changed 5 months ago by traumschule

Keywords: onionbalance added

comment:9 Changed 5 months ago by s7r

Cc: s7r added

comment:10 Changed 7 weeks ago by arma

Parent ID: #28841

comment:11 Changed 5 weeks ago by neel

Owner: set to neel
Status: newassigned

comment:12 Changed 5 weeks ago by neel

I have PRs at GitHub:

Keep in mind that:

  • There is no test, but there was no test for v2 onion services either. I don't believe it's feasible, but if needed, I will look into it and add one.
  • Descriptor IDs don't seem to exist on v3 onion services, so I only support onion service addresses.

comment:13 Changed 5 weeks ago by teor

Keywords: 040-proposed added
Milestone: Tor: unspecifiedTor: 0.4.0.x-final
Points: 0.5
Status: assignedneeds_review

I think we might want this in 0.4.0, but I am not sure.

I also think that review and fixes could take about another half day at least.

comment:14 Changed 5 weeks ago by teor

Keywords: 041-proposed added; 040-proposed removed
Milestone: Tor: 0.4.0.x-finalTor: 0.4.1.x-final

Actually, let's not try to cram things into 0.4.0.

comment:15 Changed 4 weeks ago by dgoulet

Reviewer: dgoulet

comment:16 in reply to:  12 Changed 4 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Replying to neel:

I have PRs at GitHub:

So hmmm this is quite simpler than I expected but this lack the support of SERVER= as well as DescId but that is fine because we don't have such a thing.

I don't think supporting SERVER= would be that complicated, it would basically mean add a new public API call to fetch v3 descriptor that takes an hsdir and then call directory_launch_v3_desc_fetch.

I've left comment on the PRs as well. Thanks neel!

comment:17 Changed 4 weeks ago by neel

Status: needs_revisionneeds_review

I added SERVER= support and pushed it. I also updated my torspec to add your changes. Same PRs for both tor and torspec.

comment:18 in reply to:  17 Changed 4 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Replying to neel:

I added SERVER= support and pushed it. I also updated my torspec to add your changes. Same PRs for both tor and torspec.

So fast! Thanks!

Ok another round of comments. This one is more about where to put things that I think would be better in terms of API correctness. I've tried to describe it a bit clearer what I think is the ideal path.

comment:19 Changed 4 weeks ago by neel

I don't see your comments. Could you please post them?

comment:20 Changed 4 weeks ago by neel

Status: needs_revisionneeds_review

I have made the corrections.

However, the statement you made in the GitHub torspec PR comments:

Controller app need to assert that something prefixed with v2- then 16 chars are expected, not 56.

I believe things prefixed with v2- are descriptor IDs and those are 32 bytes long.

Update: Never mind, the v2- is different from the HSAddress. I pushed the respective changes to my torspec PR.

Last edited 4 weeks ago by neel (previous) (diff)

comment:21 in reply to:  20 Changed 4 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Replying to neel:

I have made the corrections.

However, the statement you made in the GitHub torspec PR comments:

Controller app need to assert that something prefixed with v2- then 16 chars are expected, not 56.

I believe things prefixed with v2- are descriptor IDs and those are 32 bytes long.

Update: Never mind, the v2- is different from the HSAddress. I pushed the respective changes to my torspec PR.

True, my mistake.

I think you missed that comment? https://github.com/torproject/tor/pull/646#discussion_r250312545

Also, SERVER= can be specified more than once so it is very important to follow what v2 does that is a list of HSDirs, not just an rs node.

So basically, we need the entry point to be within hs_control.h API, and then calls the new function you added (hs client) but that one should take a list of HSDirs. Having the closest to the v2 behavior is desirable here and not much complexity added.

comment:22 Changed 4 weeks ago by neel

Status: needs_revisionneeds_review

I have made the changes asked for. Setting as needs review.

comment:23 in reply to:  22 Changed 3 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Replying to neel:

I have made the changes asked for. Setting as needs review.

Great! I like it. Couple comments on assert that shouldn't be there but after that we should be good to merge!

Thanks!

comment:24 Changed 3 weeks ago by neel

Status: needs_revisionneeds_review

Made the changes.

comment:25 Changed 3 weeks ago by dgoulet

Status: needs_reviewmerge_ready

Great thanks! We can go and merge this imo! Just tested it, works well! Reminder where the code and spec are. Quickly neel, can you push a quick fix on the spec comment I left. I made a mistake and it got into the spec so we just need to remove one sentence :).

Tor: ​https://github.com/torproject/tor/pull/646
Torspec: ​https://github.com/torproject/torspec/pull/52

Big thanks! Sorry for all the back and forth! :)

comment:26 Changed 3 weeks ago by neel

Removed the sentence and pushed it to my torspec.

comment:27 Changed 3 weeks ago by dgoulet

ack!

Note: See TracTickets for help on using tickets.