Opened 4 years ago

Closed 2 years ago

#17242 closed enhancement (fixed)

prop224: Implement client support

Reported by: dgoulet Owned by:
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: teor Actual Points:
Parent ID: #12424 Points: parent
Reviewer: nickm Sponsor: SponsorR-must

Description (last modified by dgoulet)

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

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

Child Tickets

TicketTypeStatusOwnerSummary
#21056defectclosedCould not pick one of the responsible hidden service directories, because we requested them all recently without success.
#21403enhancementcloseddgouletprop224: Implement HS descriptor fetching
#21855enhancementclosedprop224: Client descriptor cache
#21856enhancementclosedprop224: Client introduction point failure cache
#21857enhancementclosedprop224: Client introduction point establishment
#21858enhancementclosedprop224: Client rendezvous point establishment

Change History (27)

comment:1 Changed 4 years ago by teor

Cc: teor added
Severity: Normal

comment:2 Changed 4 years ago by teor

The revision-counter field leaks how many times the hidden service has updated its descriptor. This could identify services at the low or high end.

I wonder if we could randomise the start and increment values.

See for more detail:
https://lists.torproject.org/pipermail/tor-dev/2015-November/009936.html

comment:3 Changed 4 years ago by teor

When we encrypt the descriptor, we use a random salt.
I wonder if we could use a different salt for each replica/spread.

If we also blind the keys differently for each replica/spread, this would make it impossible to work out if descriptors are from the same hidden service, unless you can decrypt them.

See for more detail:
https://lists.torproject.org/pipermail/tor-dev/2015-November/009935.html

comment:4 in reply to:  3 Changed 4 years ago by teor

Replying to teor:

When we encrypt the descriptor, we use a random salt.
I wonder if we could use a different salt for each replica/spread.

dgoulet just mentioned on IRC that an adversary could track service intro point changes by watching how often the encrypted blob changed.

Let's use a different salt every time we repost the blob to avoid this.

comment:5 Changed 4 years ago by teor

Oh, and of course, we'd have to increment the revision-counter each time we repost, too.
Otherwise we could tell whether the introduction points for the service had really changed by watching it.

comment:6 Changed 4 years ago by teor

Please see my spec patch rend-ng-descriptors on https://github.com/teor2345/torspec.git

It adds mitigations for the above issues, each in their own commit, and mitigations for the issues I mentioned in #17239.

comment:7 Changed 4 years ago by nickm

Merged, but one thing:

Should the H(salt) you added be H("derive-salt" | salt) ? (93f47f4f4e7614d4b3debfe9b5f3a22bfe5d64b1)

And should it be explicitly said that it's truncated to 16 bytes?

comment:8 in reply to:  7 Changed 4 years ago by teor

Status: newneeds_review

Replying to nickm:

Merged, but one thing:

Should the H(salt) you added be H("derive-salt" | salt) ? (93f47f4f4e7614d4b3debfe9b5f3a22bfe5d64b1)

Since the input is random, ioerror thinks that a distinguishing value is unnecessary, and I tend to agree. I think it also has performance implications if we are hashing more than 256 bits into a 256 bit hash.

And should it be explicitly said that it's truncated to 16 bytes?

Yes, let's make that clear.

I've pushed fixups to rend-ng-descriptors on my github.
They're based on the last commit in the series to modify the relevant part of the spec, so they should fixup cleanly.

comment:9 Changed 4 years ago by teor

Status: needs_reviewnew

(Let's not change the status, as we're only reworking the spec here.)

comment:10 Changed 3 years ago by asn

Some notes from the vault:

  • What happens when prop250 gets deployed, but maybe enough dirauths are down so that the consensus does not have a shared random value? You can find here a small patch for prop224 that defines how clients should do disaster recovery when there is no shared random value in the consensus.
  • Also, in the case the Shared Random Value system screws up and causes reachability issues for clients and HSes, we should maybe introduce a consensus parameter ProbabilisticHSDirSelection that dirauths can set to off and then clients will fallback to the current HSDir selection procedure which does not require any SRVs. We could do this for a day or three, till we fix the SRV bug.
  • Finally we still have not decided on the encoding of prop224 hidden service addresses. There have been many suggestions like using the base58 encoding or the zkey encoding from GNS. Also, about chunking up the address to 5-char blocks. Also, about using some bytes of the address for metadata like Bitcoin is doing (for example, checksum, or to denote the security level of hidden services). Currently we have 3 free bits on the hidden service address, but we could also just expand the address by a byte or two to add more information if we feel it's helpful.

comment:11 Changed 3 years ago by dgoulet

Milestone: Tor: 0.2.???Tor: 0.3.0.x-final
Points: largeparent
Sponsor: SponsorR-must
Summary: Implement client and service support for proposal 224prop224: Implement client and service support

comment:12 Changed 3 years ago by dgoulet

Keywords: prop224 added
Milestone: Tor: 0.3.0.x-finalTor: 0.2.???

Aiming for this in 0.3.1+

comment:13 Changed 3 years ago by dgoulet

Description: modified (diff)
Summary: prop224: Implement client and service supportprop224: Implement client support

comment:14 Changed 3 years ago by teor

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

Milestone renamed

comment:15 Changed 3 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:16 Changed 2 years ago by dgoulet

Priority: MediumVery High

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

comment:17 Changed 2 years ago by asn

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

Triaging this into 0.3.2 after amsterdam discussion.

comment:18 Changed 2 years ago by dgoulet

prop224 tickets going in 032 for early merge. Decided after Amsterdam meeting.

comment:19 Changed 2 years ago by asn

I made an oniongit MR to do some initial code review here:
https://oniongit.eu/asn/tor/merge_requests/2/commits

comment:20 Changed 2 years ago by asn

Status: newneeds_review

OK! I'm glad to present you the first reviewable branch of client-side prop224!
You can find it in branch ticket17242_032_03!

I also opened an oniongit MR here: https://oniongit.eu/asn/tor/merge_requests/3

Please note that the first few commits of that branch (up to 3e593f0) fix various issues with the service-side code (#20657). We have currently squashed both client and service changes in the same branch so that we can test them all at once. If you'd like to only see the service-side changes to begin with, I pushed a branch ticket20657_032_05 and also opened an oniongit MR: https://oniongit.eu/asn/tor/merge_requests/4 . Let me know if you want me to make a fresh ticket for these new service-side fixes.

FWIW, there are still a few bugs here and there on the client-side code and we are actively working on finding & fixing them, so after this initial review finishes, we will have some more commits for you!

Let us know if you need help or have any questions while reviewing this branch! Cheers!

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

comment:21 Changed 2 years ago by nickm

Keywords: review-group-22 added

comment:22 Changed 2 years ago by nickm

status: I have reviewed up to but not including a3d7f53b cba99395

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

comment:23 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

Okay; I've finished the review and left comments on the merge request.

comment:24 Changed 2 years ago by nickm

Reviewer: nickm

comment:25 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

I've pushed and fixed the comment of nickm's second pass.

comment:26 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

Ok, squashed branch is in: ticket17242_032_03-squashed

Git diff with ticket17242_032_03 is empty.

Test passes as well as integration test with chutney and real network.

comment:27 Changed 2 years ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

It is upstream! Lagavulin 16 for everyone! :)

Note: See TracTickets for help on using tickets.