Opened 3 years ago

Closed 21 months ago

#20657 closed enhancement (fixed)

prop224: Implement service support.

Reported by: dgoulet Owned by: dgoulet
Priority: Very High Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, review-group-22
Cc: Actual Points:
Parent ID: #12424 Points: 6
Reviewer: nickm Sponsor: SponsorR-must

Description

This ticket is the parent one for anything related to service implementation for proposal 224.

As we break down functionalities and needed features, we'll add more child tickets.

Child Tickets

TicketTypeStatusOwnerSummary
#18054enhancementclosedprop224: Decide how to proceed with torrc options
#20524enhancementcloseddgouletRevise initial descriptor upload behavior for onion services
#20571taskcloseddgouletWhen we are really satisfied that it is right, tell protover.c about prop224 HSDir support
#20656enhancementcloseddgouletprop224: Tell protover about relay and hsdir support
#21008enhancementcloseddgouleths: Remove the introduction point private key material from hs_descriptor.h
#21888enhancementcloseddgouletprop224: Groundwork for service implementation
#21979enhancementcloseddgouletprop224: Load and configure service
#22735taskclosedasnprop224: HS desc overlap period func uses absolute times instead of slots
#22940defectclosedprop224: HS revision counter should persist after service reboot
#23157defectclosedprop224: Fix coverity reports generated by prop224 service merge (#20657)
#23311defectclosedprop224: Spammy intro point logs in v2

Change History (35)

comment:1 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:2 Changed 2 years ago by dgoulet

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

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

comment:3 Changed 2 years ago by asn

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.

Last edited 2 years ago by asn (previous) (diff)

comment:4 Changed 2 years ago by asn

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.

Last edited 2 years ago by asn (previous) (diff)

comment:5 Changed 2 years ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

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

comment:6 Changed 2 years ago by asn

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.

comment:7 Changed 2 years ago by 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.

comment:8 in reply to:  7 Changed 2 years ago by dgoulet

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

comment:9 Changed 2 years ago by 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.

Last edited 2 years ago by asn (previous) (diff)

comment:10 Changed 2 years ago by dgoulet

Priority: MediumVery High

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

comment:11 Changed 2 years ago by dgoulet

Points: parent6

comment:12 in reply to:  9 Changed 2 years ago by asn

Status: acceptedneeds_review

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.

comment:13 Changed 2 years ago by nickm

Keywords: review-group-17 added

comment:14 Changed 2 years ago by dgoulet

Keywords: review-group-17 removed
Status: needs_reviewassigned

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

comment:15 Changed 2 years ago by asn

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

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

Last edited 2 years ago by asn (previous) (diff)

comment:16 Changed 23 months ago by dgoulet

Reviewer: nickm
Status: assignedneeds_review

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 #21979. Review should start at this commit because everything before is #21979.

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

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

comment:17 Changed 23 months ago by dgoulet

Status: needs_reviewneeds_revision

Few things changed with #21979 so I'll resubmit soon a branch rebased on that with a new onion git link.

comment:18 Changed 22 months ago by dgoulet

Status: needs_revisionneeds_review

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

comment:19 Changed 22 months ago by nickm

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"

Last edited 22 months ago by nickm (previous) (diff)

comment:20 Changed 22 months ago by nickm

Status: needs_reviewneeds_revision

Review finished. :)

comment:21 Changed 22 months ago by asn

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.

comment:22 Changed 22 months ago by asn

Status: needs_revisionneeds_review

comment:23 Changed 22 months ago by nickm

Status: needs_reviewneeds_revision

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

comment:24 Changed 22 months ago by asn

Status: needs_revisionneeds_review

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

I will be testing in the meanwhile.

comment:25 Changed 22 months ago by nickm

Status: needs_reviewneeds_revision

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

comment:26 Changed 22 months ago by asn

Status: needs_revisionneeds_review

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

comment:27 Changed 22 months ago by 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!

comment:28 in reply to:  27 Changed 22 months ago by asn

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!

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

2) So yes fcdc4be is a weird one and comes straight from #23097. 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!

comment:29 Changed 22 months ago by nickm

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

comment:30 Changed 22 months ago by asn

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

comment:31 Changed 22 months ago by 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.

Last edited 22 months ago by asn (previous) (diff)

comment:32 Changed 22 months ago by nickm

Keywords: review-group-22 added

comment:33 in reply to:  31 Changed 21 months ago by dgoulet

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.

comment:34 Changed 21 months ago by dgoulet

Status: needs_reviewaccepted

Code merged but many child ticket still about some issues and sub features.

comment:35 Changed 21 months ago by dgoulet

Resolution: fixed
Status: acceptedclosed

All child ticket have been triaged. End of an era!

Note: See TracTickets for help on using tickets.