Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduce ptx package for pluggable transports dialers #373

Merged
merged 10 commits into from
Jun 14, 2021

Conversation

bassosimone
Copy link
Contributor

@bassosimone bassosimone commented Jun 9, 2021

Version 2 of the pluggable transports specification defines a function
that's like Dial() (net.Conn, error).

Because we use contexts as much as possible in probe-cli, we are
wrapping such an interface into a DialContext func.

The code for obfs4 is adapted from #341.

The code for snowflake is significantly easier than it is in
#341, because now Snowflake
supports the PTv2 spec (thanks @cohosh!).

The code for setting up a pluggable transport listener has also
been adapted from #341.

We cannot merge this code yet, because we need unit testing, yet the
newly added code already seems suitable for these use cases:

  1. testing by dialing and seeing whether we can dial (which is not
    very useful but still better than not doing it);

  2. spawning tor+pluggable transports for circumvention (we need a
    little more hammering like we did in feat: parse bridge line and use tor for circumvention #341,
    which is basically engine: we can be a snowflake pluggable transport for tor probe#1565, and then
    we will be able to do that, as demonstrated by the new, simple client which
    already allows us to use pluggable transports with tor);

  3. testing by launching tor (when available) with a set of
    pluggable transports (which depends on experiment/tor: start rewrite using new netx code probe#1596 and has not been assigned an issue yet).

Version 2 of the pluggable transports specification defines a function
that's like `Dial() (net.Conn, error`).

Because we use contexts as much as possible in `probe-cli`, we are
wrapping such an interface into a `DialContext` func.

The code for obfs4 is adapted from #341.

The code for snowflake is significantly easier than it is in
#341, because now Snowflake
supports the PTv2 spec (thanks @cohosh!).

The code for setting up a pluggable transport listener has also
been adapted from #341.

We cannot merge this code yet, because we need unit testing, yet the
newly added code already seems suitable for these use cases:

1. testing by dialing and seeing whether we can dial (which is not
very useful but still better than not doing it);

2. spawning tor+pluggable transports for circumvention (we need a
little more hammering like we did in #341,
which is basically ooni/probe#1565, and then
we will be able to do that, as demonstrated by the new, simple client which
already allows us to use pluggable transports with tor);

3. testing by launching tor (when available) with a set of
pluggable transports (which depends on https://github.com/ooni/probe-engine/issues/897
and has not been assigned an issue yet).
@bassosimone
Copy link
Contributor Author

bassosimone commented Jun 9, 2021

What is missing to land this PR:

  • test all the possible error paths (w/ unit testing if possible)
  • what interesting properties we wanna test?

(They run in 0.4s, so I think it's fine for them to always run.)
The idea is that we'll use this simpler PTDialer for testing.
@bassosimone bassosimone marked this pull request as ready for review June 10, 2021 12:24
Copy link
Contributor Author

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self code review

internal/ptx/obfs4.go Outdated Show resolved Hide resolved
internal/ptx/obfs4.go Outdated Show resolved Hide resolved
internal/ptx/obfs4_test.go Outdated Show resolved Hide resolved
internal/ptx/ptx.go Outdated Show resolved Hide resolved
internal/ptx/ptx.go Outdated Show resolved Hide resolved
internal/ptx/ptx_test.go Outdated Show resolved Hide resolved
internal/ptx/ptx_test.go Outdated Show resolved Hide resolved
internal/ptx/ptx_test.go Outdated Show resolved Hide resolved
internal/ptx/snowflake.go Outdated Show resolved Hide resolved
internal/ptx/snowflake.go Outdated Show resolved Hide resolved
internal/ptx/obfs4_test.go Outdated Show resolved Hide resolved
bassosimone and others added 2 commits June 10, 2021 18:36
Co-authored-by: Arturo Filastò <arturo@openobservatory.org>
Co-authored-by: Arturo Filastò <arturo@openobservatory.org>
Copy link
Member

@hellais hellais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a pass at reviewing this PR and could not find anything additional that needs input. I think that once we update the default bridge to be one of the ones part of TB we can merge this.

@bassosimone bassosimone merged commit 85c71c0 into master Jun 14, 2021
@bassosimone bassosimone deleted the develop branch June 14, 2021 08:20
@bassosimone
Copy link
Contributor Author

bassosimone commented Jun 14, 2021

I've opened a merge request for Snowflake that tries to address (and addresses for me) the data races I spotted when running go test -race ./... in the probe-cli repo: https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/merge_requests/42. (I am pretty happy that I was so lucky to open MR number 42 LOL.)

@cohosh
Copy link
Contributor

cohosh commented Jun 14, 2021

Hey @bassosimone thanks for opening these MRs!

cyBerta pushed a commit to cyBerta/snowflake that referenced this pull request Jun 24, 2021
The race condition occurs because concurrent goroutines are
intermixing reads and writes of `WebRTCPeer.lastReceive`.

Spotted when integrating Snowflake inside OONI in
ooni/probe-cli#373.
cyBerta pushed a commit to cyBerta/snowflake that referenced this pull request Jun 24, 2021
The race condition occurs because concurrent goroutines are intermixing
reads and writes of `WebRTCPeer.closed`.

Spotted when integrating Snowflake inside OONI in
ooni/probe-cli#373.
ainghazal pushed a commit to ainghazal/probe-cli that referenced this pull request Mar 8, 2022
* feat: introduce ptx package for pluggable transports dialers

Version 2 of the pluggable transports specification defines a function
that's like `Dial() (net.Conn, error`).

Because we use contexts as much as possible in `probe-cli`, we are
wrapping such an interface into a `DialContext` func.

The code for obfs4 is adapted from ooni#341.

The code for snowflake is significantly easier than it is in
ooni#341, because now Snowflake
supports the PTv2 spec (thanks @cohosh!).

The code for setting up a pluggable transport listener has also
been adapted from ooni#341.

We cannot merge this code yet, because we need unit testing, yet the
newly added code already seems suitable for these use cases:

1. testing by dialing and seeing whether we can dial (which is not
very useful but still better than not doing it);

2. spawning tor+pluggable transports for circumvention (we need a
little more hammering like we did in ooni#341,
which is basically ooni/probe#1565, and then
we will be able to do that, as demonstrated by the new, simple client which
already allows us to use pluggable transports with tor);

3. testing by launching tor (when available) with a set of
pluggable transports (which depends on https://github.com/ooni/probe-engine/issues/897
and has not been assigned an issue yet).

* fix: tweaks after self code-review

* feat: write quick tests for ptx/obfs4

(They run in 0.4s, so I think it's fine for them to always run.)

* feat(ptx/snowflake): write unit and integration tests

* feat: create a fake PTDialer

The idea is that we'll use this simpler PTDialer for testing.

* feat: finish writing tests for new package

* Apply suggestions from code review

* Update internal/ptx/dependencies_test.go

Co-authored-by: Arturo Filastò <arturo@openobservatory.org>

* Update internal/ptx/dependencies_test.go

Co-authored-by: Arturo Filastò <arturo@openobservatory.org>

* chore: use as testing bridge one that's used by tor browser

The previous testing bridge used to be used by tor browser but
it was subsequently removed here:

https://gitlab.torproject.org/tpo/applications/tor-browser-build/-/commit/e26e91bef8bd8d04d79bdd69f087efd808bc925d

See ooni#373 (comment)

Co-authored-by: Arturo Filastò <arturo@openobservatory.org>
ainghazal pushed a commit to ainghazal/probe-cli that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants