prop224: Implement service support.

changed milestone to %Tor: 0.3.2.x-final in legacy/trac

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

Move the 0.3.??? prop224 tickets to the 031 milestone.

Trac:
Milestone: Tor: 0.3.??? to Tor: 0.3.1.x-final

As discussed with dgoulet, I refactored the HS circuitmap codebase to be able to accept service-side circuits as well. Relay-side circuits are isolated from the service-side circuits using token types even if they have the same purpose; a test was added for this.

The public service-side API is the following, and it's identical to the relay-side one but uses origin_circuit_t instead of or_circuit_t:

void hs_circuitmap_register_intro_circ_v2_service_side(origin_circuit_t *circ, const uint8_t *digest);
void hs_circuitmap_register_intro_circ_v3_service_side(origin_circuit_t *circ, const ed25519_public_key_t *auth_key);
void hs_circuitmap_register_rend_circ_service_side(origin_circuit_t *circ, const uint8_t *cookie);
origin_circuit_t *hs_circuitmap_get_intro_circ_v3_service_side(const ed25519_public_key_t *auth_key);
origin_circuit_t *hs_circuitmap_get_intro_circ_v2_service_side(const uint8_t *digest);
origin_circuit_t *hs_circuitmap_get_rend_circ_service_side(const uint8_t *cookie);

Please see my branch circuitmap_service.

Hey David, I noticed what you said on IRC about the hash function only taking token as input and not the token_type. I think you are right on this. However, I think this does not influence the correctness of the hash table, since the hashing function is not collision resistant anyway (its output is only 4 bytes), so collisions are hanlded by putting the colliding elements in the same bucket and then running the equality function on the whole bucket (see name##_HT_FIND_P_ in src/common/ht.h). This means that if an HS is also a relay and it receives a double token, the hash table code will put both tokens in the same bucket and then it will filter the right one using hs_circuits_have_same_token(). I expanded the unittests as well to check this case (please pull).

That said, if we want to fix this in a deeper manner, we could expand hs_circuit_hash_token() by making it hash the token_type as well. I decided to not do this because I would have to do it with malloc and memcpy, and I thought it's too much of a hassle since the function is not really collision resistant in the first place.

Let me know if you think the above is wrong. I don't mind doing the changes to hs_circuit_hash_token() if you prefer that approach.

I've now been asked this twice in 2 days. For those who are curious, current development is ongoing in my branch: ticket20657_031_02.

Expect regular rebasing, HEAVY development and all the things that come with that. :)

Trac:
Owner: N/A to dgoulet
Status: new to accepted

Hello again.

I have implemented the time period functionality you asked for. Please check my branch prop224-time-period-v1.

It introduces three public functions:

+unsigned int hs_get_time_period_num(time_t now);
+unsigned int hs_get_next_time_period(time_t now);
+STATIC int descriptor_overlap_mode_is_active(const networkstatus_t *consensus);

Let me know what else you might want, or what other interface you might like.

For example, I think you asked me for a function that returns the start of a time period. Why do you want that again? I couldn't find a reason in the spec. I can of course do it if it's still useful.

Also, hope that's a useful overlap mode function for you. Let me know if you want a different interface.

I also pushed a topspec branch prop224-time-period which addresses some spec issues that we identified in IRC. Please ACK it and I will merge upstream.

Replying to asn:

I also pushed a topspec branch prop224-time-period which addresses some spec issues that we identified in IRC. Please ACK it and I will merge upstream.

Actually, after playing with the code I propose we back off on this. The get period num function needs to do some arithmetic with the period length and time_t now which if we use uint32_t, it gets unpleasant fast because time_t on x64 is on 8 bytes so we have to check for all over/underflow possibilities. Instead, if we keep it to INT_8(), we avoid this issue entirely and it makes us also much more resilient for the post-apocalypse 2038 :).

OK, I pushed my ntor branch at prop224-ntor. I also made a gitlab merge request here: https://gitlab.com/asn/tor/merge_requests/13

Please check it out, and let me know what needs to be fixed before it's merged in the rest of the service-side branch.

Also, please check out my prop224-ntor torspec branch for some basic (non-protocol-changing) improvements.

Prioritize prop224 tickets for 031 milestone. They are all "Enhancement".

Trac:
Priority: Medium to Very High

Trac:
Points: parent to 6

Replying to asn:

OK, I pushed my ntor branch at prop224-ntor. I also made a gitlab merge request here: https://gitlab.com/asn/tor/merge_requests/13

Please check it out, and let me know what needs to be fixed before it's merged in the rest of the service-side branch.

Also, please check out my prop224-ntor torspec branch for some basic (non-protocol-changing) improvements.

OK after a review from David and some comments from Nick I present the prop224-ntor-v2 branch which comes with all the code review fixes, and with a full on integration test suite similar to the ./src/test/test_ntor.sh tests for simple ntor.

It also implements the key expansion functionality as requested by David.

Putting this in needs_review just for this subtask.

Trac:
Status: accepted to needs_review

Trac:
Keywords: N/A deleted, review-group-17 added

I've moved asn's ntor branch review in legacy/trac#21750 (moved) so removing this ticket from the review group and back to the assigned state.

Trac:
Status: needs_review to assigned
Keywords: review-group-17 deleted, N/A added

Triaging this to very-early 0.3.2 based on amsterdam discussion. legacy/trac#21750 (moved) remains in 0.3.1 because it can be considered orthogonal to the rest of the branch.

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.2.x-final

Many things have been addressed from asn's ongoing review. But if we want a chance to get this upstream not in 3 months, we have to start the upstream review process. We can easily deal with multiple reviewers at once. This branch is probably not perfect nor the 100% viable product but it's a start on which we can start fixing and improving on.

Branch in: ticket20657_032_02

Based on master commit 7b236403 and legacy/trac#21979 (moved). Review should start at this commit because everything before is legacy/trac#21979 (moved).

af027ddc prop224: API for the creation of blinded keys [David Goulet]

Oniongit: https://oniongit.eu/dgoulet/tor/merge_requests/5

Trac:
Reviewer: N/A to nickm
Status: assigned to needs_review

Few things changed with legacy/trac#21979 (moved) so I'll resubmit soon a branch rebased on that with a new onion git link.

Trac:
Status: needs_review to needs_revision

The service mother branch is ready. Some stats:

 55 commits
 53 files changed, 7904 insertions(+), 1007 deletions(-)

Code: ticket20657_032_02 OnionGit: https://oniongit.eu/dgoulet/tor/merge_requests/6

Trac:
Status: needs_revision to needs_review

Note to self: I have reviewed up to but not including "prop224: Introduction circuit creation" Update: I have reviewed up to but not including "prop224: Establish rendezvous circuit for service" Update: I have reviewed up to but not including "prop224: Add service rendezvous circuit relaunch"

Review finished. :)

Trac:
Status: needs_review to needs_revision

Finished addressing Nick's review. Please check out the fixes!

The most interesting commits IMO are:

0ae3f171b * Don't set HSDir index if we don't have a live consensus.
efc6ab662 * Correctly assign HSDir flags based on protocol list
543d8a0f1 * Make ed25519 id keys optional for IPs and RPs.

Trac:
Status: needs_revision to needs_review

Reviewed again! More comments are there. Closer and closer! Any conversation on gitlab not ending with a probably needs some attention.

Trac:
Status: needs_review to needs_revision

Addressed all pending review comments again! Let me know what else is missing!

I will be testing in the meanwhile.

Trac:
Status: needs_revision to needs_review

Almost done; back into needs_revision. Just a few cleanups to make, comments to add, or tickets to open left. :)

Trac:
Status: needs_review to needs_revision

Some more fixes done and back in needs_review for now! Let me know what's next! :)

Trac:
Status: needs_revision to needs_review

13:03 <+nickm> 1) asn, are you sure you resolved all the open discussions on 
               oniongit?  It's down so I can't check for myself, but I will 
               believe you if you say "yeah I think so"
13:04 <+nickm> 2) dgoulet and asn: can you tell me a little more about 
               fcdc4bee04dd5baa3fe82bf05e6b1cc1630aa06e ?
13:04 <+nickm> I'm confused about how that patch adds and removes calls to 
               rep_hist_note_used_internal!

Replying to nickm:

{{{ 13:03 <+nickm> 1) asn, are you sure you resolved all the open discussions on oniongit? It's down so I can't check for myself, but I will believe you if you say "yeah I think so" 13:04 <+nickm> 2) dgoulet and asn: can you tell me a little more about fcdc4bee04dd5baa3fe82bf05e6b1cc1630aa06e ? 13:04 <+nickm> I'm confused about how that patch adds and removes calls to rep_hist_note_used_internal! }}}

Hey Nick!

Yes, we are done now. I just pushed one final commit (just now) which added an XXX to the trunnel definition, and now everything should have checkmark!
So yes fcdc4be is a weird one and comes straight from legacy/trac#23097 (moved). Lots of info in that ticket. Unfortunately I'm not very familiar with it either. tl;dr David was getting crazy timeouts on his circuits and it was building new internal circs every 30 secs, so he asked Mike for advice on how to use the rephist system, and that's what Mike suggested in comment:3:ticket:23097 . I'll do some double checking tomorrow on the correctness of the patch and make sure that it won't influence legacy HSes if possible.

I'll be back in this ticket tomorrow!

okay, I've squashed and merged. Please close this (or leave open) once you know what you think about fcdc4be.

So I took another look at fcdc4bee0. It's not an easy read, because the rep_hist subsystem (and what it expects from us) is quite strange.

However, the commit in question does not remove any rep_hist_note_used_internal() from any legacy call points, so the removals should not influence the legacy system.

It wouldn't surprise me if there are still circ timeout bugs like legacy/trac#23097 (moved) in the v3 system, so I expect that we will need to fine tune the rephist system further as we continue with client-side prop224 testing. For now I think we are good. I'm closing this ticket! :)

For some reason I can't close the ticket. ~~Trac just gives me a comment preview instead of closing it... If someone else can close it, please do.~~

Seems like children ticket are still open. I wonder what should we do with these.

Trac:
Keywords: N/A deleted, review-group-22 added

Replying to asn:

For some reason I can't close the ticket. ~~Trac just gives me a comment preview instead of closing it... If someone else can close it, please do.~~

Seems like children ticket are still open. I wonder what should we do with these.

So the implementation is done and merged so I propose we unparent all child ticket and treat them as "normal ticket" affecting a tor subsystem so we can close this and move on.