Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#8978 closed task (implemented)

Write server-side pluggable transport options to extra-info descriptor

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge pt wants-mocking
Cc: phw, isis@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

To correctly deploy pluggable transports that require pre-shared-secrets, like scramblesuit, we need to be able to pass the shared-secret of each bridge to BridgeDB.

This is not currently implemented, and we need to implement the ARGS option of the SMETHOD line as detailed in 180-pluggable-transport.txt.

This is a ticket for the Tor side of this task. That is, parsing the SMETHOD line and writing stuff to the extra-info descriptors.

Child Tickets

Change History (11)

comment:1 Changed 7 years ago by phw

Cc: phw added

comment:2 Changed 7 years ago by asn

Note to self: We should mention on the spec how the ARGS options and the extra-info line are escaped.

comment:3 Changed 7 years ago by asn

For the torspec side of this: Check branch bug8978 in https://git.torproject.org/user/asn/torspec.git.

It's mostly copy-paste from proposal 180, with some added information on how escaping should be done.

comment:4 Changed 7 years ago by asn

Status: newneeds_review

Please see bug8978 in https://git.torproject.org/user/asn/tor.git.

As you can see from the branch, I'm just picking up the ARGS provided by the pluggable transport and placing them directly to our extra-info descriptor. I'm not doing any validation that they are actually key=value strings for two reasons:
a) Our threat model usually trusts the transport proxy.
b) I didn't want to write an unescape function in C. We will have to write such a function anyway for BridgeDB in Python.

comment:5 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

This part looks okay, but it totally needs unit tests for the new format. (And the existing unit tests for parse_smethod_line() should really be checking the output for correctness too.)

(As a first approximation, each new or changed line of code in 0.2.5 should have test coverage. I'm not going to start actually trying to reach "100% of new lines" till after #8949 is merged, but we should try to close in on it, and not leave code untested when we could just as easily write the tests.)

comment:6 Changed 7 years ago by nickm

Keywords: wants-mocking added

comment:7 Changed 7 years ago by nickm

(And the existing unit tests for parse_smethod_line() should really be checking the output for correctness too.)

This is now #9265.

I'm rebasing onto master so I can do unit test with the new mocking/coverage stuff, in a new "bug8978_rebase" branch. My b472d964c6d8c9753393421f2fc61a3d7955e7a2 is your 642ea61bda70f2dafbd89e27aa91a5d452888358.

I'll comment again here once I've done the tests. (I'm doing tests myself on the theory that if I'm going to insist soon on test coverage, I should demonstrate that it's possible.)

comment:8 Changed 7 years ago by nickm

Status: needs_revisionneeds_review

Please look at the new tests in "bug8978_rebase". This requires some of the exposed stuff from #8929 to work, so it won't compile as-is, and we should merge it after merging that branch.

comment:9 Changed 7 years ago by asn

Added tests look good.

However, managed_proxy_create() was still marked as static (even though it was used in test_pt.c) and that caused warnings during compilation. Check my branch nickm-bug8978_rebase in https://git.torproject.org/user/asn/tor.git for a fix.

comment:10 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Squashed as "bug8978_rebase_2" and merged. Thanks!

comment:11 Changed 7 years ago by isis

Cc: isis@… added
Note: See TracTickets for help on using tickets.