prop224: Implement client support

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.

changed milestone to %Tor: 0.3.2.x-final

added component::core tor/tor milestone::Tor: 0.3.2.x-final parent::12424 points::parent priority::very high prop224 resolution::fixed review-group-22 reviewer::nickm severity::normal sponsor::R-must status::closed tor-hs type::enhancement labels

Trac:
Severity: N/A to Normal
Cc: N/A to 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

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

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.

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.

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 (moved).

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?

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.

Trac:
Status: new to needs_review

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

Trac:
Status: needs_review to new

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.

Trac:
Summary: Implement client and service support for proposal 224 to prop224: Implement client and service support
Reviewer: N/A to N/A
Milestone: Tor: 0.2.??? to Tor: 0.3.0.x-final
Sponsor: N/A to SponsorR-must
Points: large to parent

Aiming for this in 0.3.1+

Trac:
Keywords: N/A deleted, prop224 added
Milestone: Tor: 0.3.0.x-final to Tor: 0.2.???

Trac:
Summary: prop224: Implement client and service support to prop224: Implement client support
Description: Only client and service support for next generation hidden service. Be able to post/get descriptor, establish IP and RP, etc... This also should also include all control port support on both client and service side.

I'm imagining this ticket will have many child tickets in the future.

to

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.

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

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

Trac:
Priority: Medium to Very High

Triaging this into 0.3.2 after amsterdam discussion.

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

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

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

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 (moved)). 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!

Trac:
Status: new to needs_review

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

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

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

Trac:
Status: needs_review to needs_revision

Trac:
Reviewer: N/A to nickm

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to merge_ready

It is upstream! Lagavulin 16 for everyone! :)

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

mentioned in issue #17520 (moved)

mentioned in issue #17957 (moved)

mentioned in issue #18098 (moved)

mentioned in issue #18278 (moved)

mentioned in issue #20371 (moved)

mentioned in issue #21056 (moved)

mentioned in issue #21403 (moved)

mentioned in issue #21856 (moved)

mentioned in issue #21857 (moved)

mentioned in issue #21858 (moved)

mentioned in issue #22781 (moved)

mentioned in issue #23019 (moved)

mentioned in issue #23110 (moved)

mentioned in issue #23130 (moved)

mentioned in issue #23300 (moved)

mentioned in issue #23302 (moved)

mentioned in issue #23303 (moved)

mentioned in issue #23306 (moved)

mentioned in issue #23307 (moved)

mentioned in issue #23308 (moved)