hsv3: Support onionbalance keys when handling INTRO2 cells

We have encountered a small issue with onionbalance viability for v3s.

While the descriptor cross-certificates are no longer an issue (legacy/trac#29583 (moved)), there is an issue with the ntor handshake during the INTRODUCE1/INTRODUCE2 handshake between the client and service.

In particular, as specified in rend-spec-v3.txt [NTOR-WITH-EXTRA-DATA] the subcredential (which includes the onion address) is used as part of the ntor key material to generate end-to-end encryption keys and MAC keys so that the service can communicate with the client:

   To make an INTRODUCE1 cell, the client must know a public encryption
   key B for the hidden service on this introduction circuit. The client
   generates a single-use keypair:
             x,X = KEYGEN()
   and computes:
             intro_secret_hs_input = EXP(B,x) | AUTH_KEY | X | B | PROTOID
             info = m_hsexpand | subcredential
             hs_keys = KDF(intro_secret_hs_input | t_hsenc | info, S_KEY_LEN+MAC_LEN)
             ENC_KEY = hs_keys[0:S_KEY_LEN]
             MAC_KEY = hs_keys[S_KEY_LEN:S_KEY_LEN+MAC_KEY_LEN]

The issue here is that when the client prepares the INTRO1 cell, it uses the subcredential of the frontend OBv3 service, but the INTRO2 cell actually goes to a backend OBv3 instance which has a different subcredential. This causes the backend instance to not be able to verify the MAC of the cell, and generally finish the ntor handshake....

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

Trac:
Parent Ticket: legacy/trac#26768 (moved)

added 044-must in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.4.4.x-final in Legacy / Trac network-team-roadmap-2020Q1 in Legacy / Trac onionbalance in Legacy / Trac owner::asn in Legacy / Trac parent::26768 in Legacy / Trac points::5 in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac reviewer::dgoulet in Legacy / Trac reviewer::nickm in Legacy / Trac scaling in Legacy / Trac severity::normal in Legacy / Trac sponsor::27-must in Legacy / Trac status::closed in Legacy / Trac tor-hs in Legacy / Trac tor-spec in Legacy / Trac type::enhancement in Legacy / Trac labels

Trac:
Reviewer: N/A to dgoulet
Owner: N/A to asn
Status: new to assigned

The suggested approach here is to introduce a torrc option for the OB instances HiddenServiceOnionbalanceFrontendConfigFile that points to a file which contains info about the OB frontend. In particular, it will contain (a number of) FrontendOnionAddress which is the onion address of the frontend, and can be used to decrypt and finish the ntor handshake with the client.

It's still not decided if we should allow the instances with that configured to also be accessed over their regular instance onion or not.

I will be working on this and hopefully have something before the end of week. Unittests might be hard to do.

Trac:
Keywords: tor-hs scaling onionbalance network-team-roadmap-september tor-spec deleted, tor-hs scaling onionbalance tor-spec network-team-roadmap-2020Q1 added

Will take over this for asn. We just had a conf call about it.

Trac:
Status: assigned to accepted
Owner: asn to dgoulet

Branch to be tested: ticket32709_043_01 PR: https://github.com/torproject/tor/pull/1647

I have done ZERO test on it.

Trac:
Status: accepted to needs_review

Hey hey,

this looks really good. I actually tested this (and after fixing a few issues) I managed to connect to the backend service and exchange data over an onionbalance descriptor! That's a first!

I pointed a few issues on GH. The main ones is an off-topic assertion failure on the logging subsystem, and the more important one is something weird going on with the hs_cell_parse_introduce2 data structure and client_pk gets wiped when it shouldn't. I fixed both of those issues quickly for testing but we need to fix them well. Ah, also the time period iteration feature is missing, and I made a comment to that effect.

Other than that, it looks good, plus it needs changes file and man page entry. Also, a unittest for the config parsing wouldn't be bad if possible.

Thanks a lot!

Trac:
Status: needs_review to needs_revision

Trac:
Keywords: tor-hs scaling onionbalance tor-spec network-team-roadmap-2020Q1 deleted, tor-hs scaling onionbalance tor-spec network-team-roadmap-2020Q1 043-must added

Hi! I'm going to take a look at the code in detail, but for now, some top-level things, some of which we already talked about on IRC:

• It needs unit tests
• CI needs to pass: it looks like practracker is telling us to break up config_service_v3() into manageable pieces
• I think we need a corresponding patch to rend-spec-v3.txt
• We should talk a bit about whether we want to accept both possible subcredentials or only one.

Trac:
Keywords: tor-hs scaling onionbalance tor-spec network-team-roadmap-2020Q1 043-must deleted, tor-hs scaling onionbalance tor-spec network-team-roadmap-2020Q1 043-must postfreeze-ok added

PR and branch are still the same but totally updated/changed after initial review from nickm/asn.

Currently with unit tests and practracker happy. Spec patch is being written by asn with a justification for the usage of both subcredentials.

I've notified asn about this so he can continue OBv3 testing with this branch.

Trac:
Status: needs_revision to needs_information

OK did a review on the branch and posted on the PR. Been testing it on OBv3 the past few hours and it works just fine!

As discussed with David, I also wrote a spec branch documenting and analyzing the behavior introduced by this code: https://github.com/asn-d6/torspec/tree/bug32709 Please let me know if something is missing and should be added.

Trac:
Status: needs_information to needs_revision

Trac:
Reviewer: dgoulet to nickm
Status: needs_revision to needs_review

Okay, reading through it all again. I'll make comments in the PR, but here are some higher-level questions.

1. Does it really make sense to compute the subcredential for three time periods? It seems to me that we are never close to more than two periods. Maybe we should calculate the subcredentials for "now" and "the closest period other than now".

2. The performance here is going to be needlessly bad. Keep in mind that every time we call hs_ntor_client_get_introduce1_keys(), we're doing a curve25519 calculation... but the curve25519 calculation will be the same here every time! The only input that changes is the subcredential, which is an input only to the XOF() part of the process. This could be a followup branch, I guess, though.

3. We should think about timing side channels here. This could also be a followup, however.

4. Test coverage on these changes should really be higher.

Trac:
Status: needs_review to needs_revision

We talked about this on IRC; here's the current plan as I understand it.

We're going to aim this ticket for 0.4.4, since doing it properly will require more time than we have for 0.4.3. We'll plan to open the merge window for 0.4.4 early, for this sponsor only.

Between today and next Wednsday or so, I'll do a branch based on this one that will: a) turn subcredential into a proper type b) implement a constant-time multi-subcredential backend solution for items 2 and 3 above, on top of this branch. c) refactor config_service_v3() a little

After that, for minimal viable product, we need: d) improved test coverage, e) to stop recomputing the subcredentials for every request

Later on, we can make these improvements: f) compute fewer subcredentials

Trac:
Points: 2 to 5
Keywords: tor-hs scaling onionbalance tor-spec network-team-roadmap-2020Q1 043-must postfreeze-ok deleted, tor-hs scaling onionbalance tor-spec network-team-roadmap-2020Q1 044-must added
Milestone: Tor: 0.4.3.x-final to Tor: 0.4.4.x-final

I have done (a) and (b) above; I'll get to (c) before Wednesday.

I'm putting my work in progress at ticket32709_044_01 with PR at https://github.com/torproject/tor/pull/1672 in case you want to take a look. It conflicts with the merged legacy/trac#32137 (moved), so CI isn't running on it for the moment.

Trac:
Status: needs_revision to assigned
Owner: dgoulet to asn
Reviewer: nickm to nickm, dgoulet

I've started work on (c), but I think it's better to do in a separate branch. I've opened legacy/trac#33014 (moved) for it.

Per request, I've rebased this as a new branch ticket32709_044_02 with PR at https://github.com/torproject/tor/pull/1679 . (This one won't have a merge conflict with legacy/trac#32137 (moved).)

I noticed a few places on the pull request where comments need to be clarified, or documentation needs to be added.

I accidentally did the old pull request as well. Sorry!

Hello,

please see https://github.com/torproject/tor/pull/1685 for a PR that fixes the remaining issues of comment:15 and also fixes the comment/docs issues raised by teor.

Trac:
Status: assigned to needs_review

Some comments on the PR.

Looks like travis CI is failing due to some bad HTML in the doxygen.

I'm reviewing asn's changes now.

Okay, I've left some comments on the PR.

Trac:
Status: needs_review to needs_revision

Trac:
Cc: s7r, gk to s7r, gk, mikeperry

Pushed fix to the comments. Let me know how you like that! :)

Trac:
Status: needs_revision to needs_review

Ok I've found a tiny syntax thing but I believe we can merge this in 044.

arg. I went to the PR and found that I had uncommitted comments. I've put my comments on the PR. Do they make sense?

Trac:
Status: needs_review to needs_revision

Pushed fix commit that hopefully addresses unresolved comments! Thanks for review!

Trac:
Status: needs_revision to needs_review

Left a small comment. Other than that, looks good to go!

Trac:
Status: needs_review to needs_revision

Did fixes and added changes/man page! Thanks for reviews!

Trac:
Status: needs_revision to needs_review

Okay, I think that's everything. Merging this to master! Nice work, everybody!

Trac:
Status: needs_review to closed
Resolution: N/A to implemented

closed

changed time estimate to 40h

mentioned in issue legacy/trac#33014 (moved)

moved from legacy/trac#32709 (moved)

added Feature + 1 deleted label and removed 1 deleted label

added Specifications + 1 deleted label and removed 1 deleted label

added Onion Services label and removed 1 deleted label