prop224: Implement HSDir support
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.
Activity
changed milestone to %Tor: 0.3.0.x-final
added TorCoreTeam201611 actualpoints::6+ actualreviewpoints=2 component::core tor/tor milestone::Tor: 0.3.0.x-final nickm-deferred-20161005 owner::dgoulet parent::12424 points::parent priority::high prop224 resolution::fixed review-group-11 reviewer::asn severity::normal sponsor::R-must status::closed tor-hs type::enhancement labels
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 (moved), #19024 (moved), #18571 (moved), #18572 (moved), #19205 (moved)
Branch is
ticket17238_029_01. Merge request here:https://gitlab.com/dgoulet/tor/merge_requests/9
Trac:
Status: accepted to needs_review
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 (moved), #19024 (moved), #18571 (moved), #18572 (moved), #19205 (moved)
Branch is
ticket17238_029_01. Merge request here: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.5so far)-
Please see brach
ticket17238_029_11in my repo.It features:
- It adds support for fetching descs from HSDirs (using "/tor/hs/3/" 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.
-
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
fetchfeature 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 (moved)). This is imo ready for a
merge_readystate 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%Trac:
Status: needs_revision to needs_information -
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
fetchfeature 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 (moved)). This is imo ready for a
merge_readystate 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?Trac:
Status: needs_information to needs_revision -
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.
-
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_02Trac:
Status: needs_revision to needs_information -
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_02Trac:
Status: needs_information to merge_ready
Actualpoints: N/A to 6+ 
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?
Trac:
Status: merge_ready to needs_revisionActual-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.
-
This should be in
needs_reviewas I've replied to teor and nickm's review.https://gitlab.com/dgoulet/tor/merge_requests/10
Trac:
Status: needs_revision to needs_review mentioned in issue #18515 (moved)
mentioned in issue #18571 (moved)
mentioned in issue #18572 (moved)
mentioned in issue #19024 (moved)
mentioned in issue #19024 (moved)
mentioned in issue #19043 (moved)
mentioned in issue #19205 (moved)
mentioned in issue #19642 (moved)
