prop224: client can pick super old rendezvous points

We just discovered that we dont enforce protover rules when prop224 clients pick rendezvous points, so we end up picking rps on 0.2.8 which make our circuits hang.

changed milestone to %Tor: 0.3.2.x-final

added component::core tor/tor milestone::Tor: 0.3.2.x-final priority::high prop224 resolution::fixed reviewer::asn severity::normal sponsor::R-can status::closed tor-hs type::defect labels

See branch: bug23361_032_01

The change it not that trivial but I hope simple enough.

Trac:
Reviewer: N/A to asn
Status: new to needs_review

Patch does not handle cannibalized circs correctly. Cannibalizable circs to non-v3-supporting exits should not be used for v3 rend.

Trac:
Status: needs_review to needs_revision

You might also find it useful to put a warn message in, if you're about to send a v3 rend request to a relay that doesn't support a v3 rend request. In case there are other edge cases (or new ones show up in the future).

This also needs a spec change. (Or maybe I missed the relevant part in the spec.)

Fixup branch pushed at bug23361_032_01 in my repo!

Contains two fixup commits: One fixes the hsv3 circuit detection, and the second regulates cannibalization for hsv3 rp circuits.

Let me know if you have any questions. Thanks!

Trac:
Status: needs_revision to needs_review

• uncanibalizable --> I think missing a n.

• Not sure about this log info: log_info(LD_GENERAL, "Getting v3 rp circuit!");

Rest lgtm;

Trac:
Status: needs_review to needs_revision

Doc fixes done, and force pushed branch.

Trac:
Status: needs_revision to needs_review

lgtm;

Trac:
Status: needs_review to merge_ready

Code looks okay.

This still needs the spec change that teor mentioned above, right?

Also, this seems to violate proposal 224 section 4.3, which says that we can use older rendezvous points. Why did we decide not to do that?

Trac:
Status: merge_ready to needs_revision

Replying to nickm:

Also, this seems to violate proposal 224 section 4.3, which says that we can use older rendezvous points. Why did we decide not to do that?

That is a spec issue that needs to be updated. Back at the Montreal hidden service meeting, we realized that we needed legacy rendezvous point to relay an HS cell that had more bytes than the 20 bytes rendezvous cookie and that patch got in 0.2.9 (commit be0e1e9e2f6). So, HS client can not use RPs below that version which is HSRend=2.

HOWEVER, we should most certainly pad all RENDEZVOUS cells in the legacy HS system so v2 and v3 cells look "alike".

Spec patch can be found in bug23361 in my repo. Putting this back in needs_review.

I also made #23420 (moved) for the padding feature that David mentioned in comment:11.

Trac:
Status: needs_revision to needs_review

Trac:
Status: needs_review to merge_ready

Okay; I merged the spec branch, but I get a pile of warnings when I try to merge it. It looks like there was a runaway comment.

I'm fine with just fixing the comment and merging, but if it's been unbuildable, that means that nobody has has tried testing the code, right?

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

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

Trac:
Status: reopened to needs_revision

Ouch. Seems like I left a bad comment in there.

Pushed a fixup that addresses it.

Let me know if you want me to rebase the whole branch on master.

Trac:
Status: needs_revision to merge_ready

OK I tested this branch again to make sure that nothing broke in the meanwhile. Branch seems to work well.

To test, I used a client with this branch, connected to an hsv3 service and made sure that the rendezvous point used supported the hsv3 protocol and data made it to the service. I did this test about 20 times to make sure that we only pick legit RPs.

I'm always afraid whenever there's a chance that what we're testing has diverged from what we're running--so, thanks for double-checking that we actually have been testing this code.

Squashed and merging to 0.3.2.

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

closed

moved to tpo/core/tor#23361 (closed)