HSFETCH support for v3 Hidden Services

changed milestone to %Tor: 0.4.1.x-final

added 041-proposed component::core tor/tor milestone::Tor: 0.4.1.x-final onionbalance owner::neel parent::28841 points::0.5 priority::medium prop224-extra resolution::fixed reviewer::dgoulet severity::normal status::closed tor-control tor-hs type::defect labels

Trac:
Milestone: N/A to Tor: unspecified
Keywords: N/A deleted, tor-control, tor-hs added

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

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.)

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

Tagging with prop224-extra in the interim until we know for sure whether its needed. See tor-dev email thread, notably [[https://lists.torproject.org/pipermail/tor-dev/2018-April/013137.html|this]].

Trac:
Keywords: N/A deleted, prop224-extra added
Cc: N/A to dmr

Trac:
Cc: dmr to dmr, neel@neelc.org
Owner: N/A to neel
Status: new to assigned

Trac:
Cc: dmr, neel@neelc.org to dmr
Owner: neel to N/A

Trac:
Status: assigned to new

Trac:
Keywords: N/A deleted, onionbalance added

Trac:
Cc: dmr to dmr, s7r

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

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

I have PRs at GitHub:

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

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.

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.

Trac:
Milestone: Tor: unspecified to Tor: 0.4.0.x-final
Keywords: N/A deleted, 040-proposed added
Status: assigned to needs_review
Points: N/A to 0.5

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

Trac:
Milestone: Tor: 0.4.0.x-final to Tor: 0.4.1.x-final
Keywords: 040-proposed deleted, 041-proposed added

Trac:
Reviewer: N/A to dgoulet

Replying to neel:

I have PRs at GitHub:

Tor: https://github.com/torproject/tor/pull/646

Torspec: https://github.com/torproject/torspec/pull/52

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!

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision

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

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.

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

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!

Trac:
Status: needs_review to needs_revision

Made the changes.

Trac:
Status: needs_revision to needs_review

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! :)

Trac:
Status: needs_review to merge_ready