Opened 22 months ago

Closed 9 months ago

#17238 closed enhancement (fixed)

prop224: Implement HSDir support

Reported by: dgoulet Owned by: dgoulet
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, actualreviewpoints=2, nickm-deferred-20161005, review-group-11, TorCoreTeam201611
Cc: asn Actual Points: 6+
Parent ID: #12424 Points: parent
Reviewer: asn Sponsor: SponsorR-must

Description

This is part of the grand proposal 224. The task here is to implement the new descriptor format, HSDir caching for store/lookup and make sure relay can serve them. Note that this ticket does NOT require the client and service to support the new descriptor format. This is only on the relay side.

Child Tickets

TicketTypeStatusOwnerSummary
#18515enhancementcloseddgouletRefactoring routerparse.c/.h, hidden services parser in a seperate file
#18571enhancementcloseddgouletprop224: Encode/Decode descriptor implementation
#18572enhancementcloseddgouletprop224: HSDir descriptor cache implementation
#19024enhancementcloseddgouletprop224: Refactor rend_data_t so be able to use multiple HS version
#19205enhancementcloseddgouletprop224: HSDir fetch/store implementation
#19649enhancementclosedDefine a link specifier containing an ntor onion key
#19972defectclosedprop224: Proposal fixes from implementation of HSDir support

Change History (39)

comment:1 Changed 16 months ago by asn

  • Cc asn added
  • Severity set to Normal
  • Sponsor set to None

comment:2 Changed 16 months ago by dgoulet

  • Milestone changed from Tor: 0.2.??? to Tor: 0.2.9.x-final
  • Priority changed from Medium to High
  • Sponsor changed from None to SponsorR-must

comment:3 Changed 16 months ago by dgoulet

  • Keywords prop224 added
  • Summary changed from Implement HSDir support for proposal 224 to prop224: Implement HSDir support

comment:4 Changed 14 months ago by isabela

  • Points changed from large to 6

comment:5 Changed 14 months ago by dgoulet

  • Points 6 deleted

comment:6 Changed 13 months ago by nickm

  • Points set to parent

comment:7 Changed 12 months ago by dgoulet

  • Keywords TorCoreTeam201608 added

comment:8 Changed 12 months ago by dgoulet

  • Owner set to dgoulet
  • Status changed from new to accepted

comment:9 follow-up: Changed 11 months ago by dgoulet

  • Status changed from accepted to needs_review

Ok _big_ branch here. Some of it has been reviewed already in different branches but this one bring the whole feature together. Unfortunately, we need a single branch because all child tickets depends on each other.

The branch contains basically in order of commit appearance: #18515, #19024, #18571, #18572, #19205

Branch is ticket17238_029_01. Merge request here:

https://gitlab.com/dgoulet/tor/merge_requests/9

comment:10 Changed 11 months ago by nickm

  • Keywords review-group-7 added

comment:11 Changed 11 months ago by asn

  • Reviewer set to asn

comment:12 in reply to: ↑ 9 Changed 11 months ago by asn

Replying to dgoulet:

Ok _big_ branch here. Some of it has been reviewed already in different branches but this one bring the whole feature together. Unfortunately, we need a single branch because all child tickets depends on each other.

The branch contains basically in order of commit appearance: #18515, #19024, #18571, #18572, #19205

Branch is ticket17238_029_01. Merge request here:

https://gitlab.com/dgoulet/tor/merge_requests/9

Hello.

Did an initial gitlab review here of the following commits:

250cde9 * prop224: Descriptor decoding implementation
394266f * prop224: Descriptor encoding implementation
57084a4 * prop224: Add new cert type for hidden service
61041e2 * trunnel: Uncomment link_specifier so we can use it
ceaf43b * Move token parsing code to parsecommon.{c|h}

I will review those commits more ASAP, and also move on to the other commits.

(Also actual_review_points == 0.5 so far)

comment:13 Changed 11 months ago by asn

OK today I addressed all of David's review comments, and also did an initial review of ec4afbf.

More review coming soon.

(actual_review_points == 1.5)

comment:14 Changed 11 months ago by asn

  • Keywords actualreviewpoints=2 added
  • Status changed from needs_review to needs_revision

comment:15 Changed 11 months ago by dgoulet

I just pushed the latest branch in ticket17238_029_01 (contains all the fixup commit) and the rebased version in ticket17238_029_02.

asn has the mutex on that branch for some fixes so keeping under needs_revision.

comment:16 Changed 11 months ago by asn

Please see brach ticket17238_029_11 in my repo.

It features:

  • It adds support for fetching descs from HSDirs (using "/tor/hs/3/<id>" path)
  • It adds high-level unittests for pushing/fetching descs from HSDirs.
  • It introduces a prefix for desc signatures.
  • It's rebased on latest git master.
  • It fixes a few bugs and improves some code.
  • Addresses check-spaces (although in its own commit).

David another thing we should discuss is whether we should guard all this HSDir cache code with a consensus parameter, or we should just leave it there active but unused till we deploy the rest of the prop224 parts.

comment:17 follow-up: Changed 11 months ago by dgoulet

  • Status changed from needs_revision to needs_information

I've merged all your things! _minor_ fixes here and there mostly to follow some syntax but that's it. I've integrated them in the current commit that exists. The fetch feature has its own commit as well as the unit test for it. Finally, the "make check-spaces happy" is also its own commit.

On top of all this, I've added a commit for the consensus params (discussed in #19899). This is imo ready for a merge_ready state that is put it in nickm's court.

@asn, can you confirm your happiness? :) Once you do, I'll create a merge request on Gitlab. Located in branch ticket17238_029_02.

Test code coverage:

src/or/hs_cache.c      - 100%
src/or/hs_common.c     - 82%     /* Existing code refactored in there. *
src/or/hs_descriptor.c - 90.7%
Last edited 11 months ago by dgoulet (previous) (diff)

comment:18 in reply to: ↑ 17 Changed 11 months ago by asn

  • Status changed from needs_information to needs_revision

Replying to dgoulet:

I've merged all your things! _minor_ fixes here and there mostly to follow some syntax but that's it. I've integrated them in the current commit that exists. The fetch feature has its own commit as well as the unit test for it. Finally, the "make check-spaces happy" is also its own commit.

On top of all this, I've added a commit for the consensus params (discussed in #19899). This is imo ready for a merge_ready state that is put it in nickm's court.

Hmm, you mean af4d413?

I don't see a consensus param there. I only see a torrc option being introduced.

I don't think a torrc option is that useful: I'd actually like a real consensus param (like SRVAgreements). Am I reading this right?

comment:19 Changed 11 months ago by asn

Regarding HS stats, maybe let's find some time to discuss this during the dev meeting, and see what more people think? Specifically, should we add a new stats line for next gen onion services, or blend them with the old stats?

The former approach sounds more reasonable, but we should think of security issues here (gathering stats on a growing community) and also whether the current noise will make the stats useless until there are hundreds of next gen onion services.

A patch to add stats would be pretty small and easy whichever approach we follow, so we can also get it through in the next release.

comment:20 Changed 11 months ago by dgoulet

  • Status changed from needs_revision to needs_information

re consensus param: Yes I did fail there. I just pushed a new branch with the top commit fixed. It should be a bit better now :).

re stats: Agree, we could also put some researchers in the loop! So I say let's postpone until dev meeting and anyway even if this gets in 029, the feature won't be enabled because of the consensus param.

See branch: ticket17238_029_02

comment:21 Changed 11 months ago by dgoulet

  • Actual Points set to 6+
  • Status changed from needs_information to merge_ready

Ok! We think (asn and I) this is ready for the next stage, the nickm stage! :)

https://gitlab.com/dgoulet/tor/merge_requests/10

Branch: ticket17238_029_02

comment:22 Changed 11 months ago by dgoulet

  • Keywords TorCoreTeam201609 added; TorCoreTeam201608 removed

Review and merge will happen in September.

comment:23 Changed 11 months ago by nickm

On gitlab I reviewed the commits up to and including "prop224: Descriptor decoding implementation". More to come.

Actual-review-points += .3

comment:24 Changed 11 months ago by nickm

  • Status changed from merge_ready to needs_revision

Actual-review-points += .2

I've been over the whole branch on gitlab now.

Remember, please no rebasing yet!

Here are some things that I did *not* check:

  • Are there memory leaks in the unit tests?
  • Does the code compile with all warnings turned on?
  • Do the unit tests pass with --enable-expensive-coverage
  • Does the encoded/decoded format match prop224 exactly?

comment:25 Changed 11 months ago by nickm

  • Keywords review-group-8 added; review-group-7 removed

comment:26 Changed 11 months ago by teor

Actual-review-points += .5

I've looked at some of this code, and made suggestions for improvements on gitlab.

If the unit tests are comprehensive enough to exercise all the packing code, I'd like to make sure we test it on a big-endian system before merging, but those can be hard to come by these days.

comment:27 Changed 10 months ago by nickm

  • Keywords review-group-9 added; review-group-8 removed

comment:28 Changed 10 months ago by nickm

  • Keywords nickm-deferred-20161005 added
  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.3.0.x-final

Deferring big/risky-feature things (even the ones I really love!) to 0.3.0. Please argue if I'm wrong.

comment:29 Changed 10 months ago by nickm

These have sat in needs_revision for a few weeks at least. Removing from review-group-9, not adding to review-group-10.

comment:30 Changed 9 months ago by dgoulet

  • Status changed from needs_revision to needs_review

This should be in needs_review as I've replied to teor and nickm's review.

https://gitlab.com/dgoulet/tor/merge_requests/10

comment:31 Changed 9 months ago by nickm

  • Keywords review-group-9 removed

comment:32 Changed 9 months ago by asn

  • Keywords TorCoreTeam201610 added; TorCoreTeam201609 removed

comment:33 Changed 9 months ago by nickm

  • Keywords review-group-11 added

comment:34 Changed 9 months ago by dgoulet

  • Keywords TorCoreTeam201611 added; TorCoreTeam201610 removed

comment:35 Changed 9 months ago by nickm

I've had another pass through. This looks very close to done.

comment:36 Changed 9 months ago by nickm

  • Status changed from needs_review to needs_revision

comment:37 Changed 9 months ago by dgoulet

  • Status changed from needs_revision to needs_review

Resquash branch: ticket17238_029_02-resquash

Bunch of tickets needs to be opened once this is merged upstream. I will take care of this.

comment:38 Changed 9 months ago by nickm

Merged, with many bugfixes with help from dgoulet!!!1!

comment:39 Changed 9 months ago by dgoulet

  • Resolution set to fixed
  • Status changed from needs_review to closed

Great success! Thanks to everyone who worked on this! Really a team effort! :)

Many other tickets will be opened soon to address various issues with current implementation but for now, we are done with this one :D.

Note: See TracTickets for help on using tickets.