With this factory, we want to construct ourselves the TLS dialer
so that we can use a dialer wrapper that always sets timeouts when
reading, addressing https://github.com/ooni/probe/issues/1609.
As a result, we cannot immediately replace the i/e/netx factory
for creating a new HTTP transport, since the functions signatures
are not directly compatible.
Refactoring is part of https://github.com/ooni/probe/issues/1505.
This diff is part of https://github.com/ooni/probe/issues/1505.
You will notice that I have not adapted all the (great) tests we had
previously. They should live at another layer, and namely the one that
deals with performing measurements.
When I'm refactoring such a layer I'll ensure those tests that I have
not adapted here are reintroduced into the tree.
Auto-configure every relevant TLS field as close as possible to
where it's actually used.
As a side effect, add support for mocking the creation of a TLS
connection, which should possibly be useful for uTLS?
Work that is part of https://github.com/ooni/probe/issues/1505
The BogonResolver relied on its wrapper resolver to pass along the
list of addresses _and_ the error. But the idiomatic thing to do is
often to return `nil` when there is an error.
I broke this very fragile assumption in https://github.com/ooni/probe-cli/pull/399.
I could of course fix it, but this assumption is clearly wrong
and we should not allow such fragile code in the tree.
We are not using BogonIsError much in the tree. The only place in
which we're using it for measuring seems to be dnscheck.
It may be that this surprising behavior was what caused the issue at
https://github.com/ooni/probe/issues/1510 in the first place.
Regardless, let's remove fragile code and adjust the test that was
failing. Also that test is quick so it can run in `-short` mode.
Spotted while working on https://github.com/ooni/probe/issues/1505.
* refactor(netxlite): make sure we always use netmocks
While there, improve logging and make sure we test 100% of the
package with unit tests only. (We don't need to have integration
testing in this package because it's fairly simple/obvious.)
Part of https://github.com/ooni/probe/issues/1505
* further improve logs
What do I mean by pivoting? Netx is currently organized by row:
```
| dialer | quicdialer | resolver | ...
saving | | | | ...
errorwrapping | | | | ...
logging | | | | ...
mocking/sys | | | | ...
```
Every row needs to implement saving, errorwrapping, logging, mocking (or
adapting to the system or to some underlying library).
This causes cross package dependencies and, in turn, complexity. For
example, we need the `trace` package for supporting saving.
And `dialer`, `quickdialer`, et al. need to depend on such a package.
The same goes for errorwrapping.
This arrangement further complicates testing. For example, I am
currently working on https://github.com/ooni/probe/issues/1505 and
I realize it need to repeat integration tests in multiple places.
Let's say instead we pivot the above matrix as follows:
```
| saving | errorwrapping | logging | ...
dialer | | | | ...
quicdialer | | | | ...
logging | | | | ...
mocking/sys | | | | ...
...
```
In this way, now every row contains everything related to a specific
action to perform. We can now share code without relying on extra
support packages. What's more, we can write tests and, judding from
the way in which things are made, it seems we only need integration
testing in `errorwrapping` because it's where data quality matters
whereas, in all other cases, unit testing is fine.
I am going, therefore, to proceed with these changes and "pivot"
`netx`. Hopefully, it won't be too painful.
We are not using them anymore. The only nettest still using the
legacy netx implementation is tor, for which setting these fields
is useless, because it performs each measurement into a separate
goroutine. Hence, let us start removing this part of the legacy
netx codebase, which is hampering progress in other areas.
Occurred to me while doing testing for the recent changes in
error mapping (https://github.com/ooni/probe/issues/1505).
* refactor: move scrubbingLogger to the scrubber pkg
We need it exported so we can use it in the new implementation.
Part of https://github.com/ooni/probe/issues/1687
* fix test
* refactor: move bytecounting conn in bytecounter pkg
This enables other pieces of code to request bytecounting without
depending on netx or on the perverse using-the-context-to-configure-
byte-counting mechanism.
Also occurred when working on https://github.com/ooni/probe/issues/1687
* fix: add missing docs
The current implementation assumes the user has already installed tor
on the current system. If tor is not present, the experiment fails.
This is meant to be the first version of this experiment.
We are going to add more functionality in subsequent revisions of
this experiment, once we've collected more feedback.
Reference issue: https://github.com/ooni/probe/issues/1565.
Here's the spec PR: https://github.com/ooni/spec/pull/218.
Here's the issue tracking future work: https://github.com/ooni/probe/issues/1686
This is a very light refactoring of the mlablocatev2 package where we do
the following things:
1. use interfaces rather than depending on other pkgs where possible
2. add a missing test to the test suite
3. write more comprehensive docs (including todo-next comments)
This is a very light refactoring of the mlablocate package where we do
the following things:
1. use interfaces rather depending on other pkgs where possible
2. only keep the fields we really need in the result struct
3. write more comprehensive docs (including todo-next comments)
While there, use `neubot/dash` rather than `ndt7` for the tests.
* fix(all): introduce and use iox.CopyContext
This PR is part of https://github.com/ooni/probe/issues/1417.
In https://github.com/ooni/probe-cli/pull/379 we introduced a context
aware wrapper for io.ReadAll (formerly ioutil.ReadAll).
Here we introduce a context aware wrapper for io.Copy.
* fix(humanize): more significant digits
* fix: rename humanize files to follow the common pattern
* fix aligment
* fix test
* fix(all): introduce and use iox.ReadAllContext
This improvement over the ioutil.ReadAll utility returns early
if the context expires. This enables us to unblock stuck code in
case there's censorship confounding the TCP stack.
See https://github.com/ooni/probe/issues/1417.
Compared to the functionality postulated in the above mentioned
issue, I choose to be more generic and separate limiting the
maximum body size (not implemented here) from using the context
to return early when reading a body (or any other reader).
After implementing iox.ReadAllContext, I made sure we always
use it everywhere in the tree instead of ioutil.ReadAll.
This includes many parts of the codebase where in theory we don't
need iox.ReadAllContext. Though, changing all the places makes
checking whether we're not using ioutil.ReadAll where we should
not be using it easy: `git grep` should return no lines.
* Update internal/iox/iox_test.go
* fix(ndt7): treat context errors as non-errors
The rationale is explained by the comment documenting reduceErr.
* Update internal/engine/experiment/ndt7/download.go
* 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 https://github.com/ooni/probe-cli/pull/341.
The code for snowflake is significantly easier than it is in
https://github.com/ooni/probe-cli/pull/341, because now Snowflake
supports the PTv2 spec (thanks @cohosh!).
The code for setting up a pluggable transport listener has also
been adapted from https://github.com/ooni/probe-cli/pull/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 https://github.com/ooni/probe-cli/pull/341,
which is basically https://github.com/ooni/probe/issues/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:
e26e91bef8
See https://github.com/ooni/probe-cli/pull/373#discussion_r649820724
Co-authored-by: Arturo Filastò <arturo@openobservatory.org>
* refactor(netx/dialer): hide implementation complexity
This follows the blueprint of `module.Config` and `nodule.New`
described at https://github.com/ooni/probe/issues/1591.
* fix: ndt7 bug where we were not using the right resolver
* fix(legacy/netx): clarify irrelevant implementation change
* fix: improve comments
* fix(hhfm): do not use dialer.New b/c it breaks it
Unclear to me why this is happening. Still, improve upon the
previous situation by adding a timeout.
It does not seem a priority to look into this issue now.
The socks5 factory always returns a DialContext capable dialer. We just
need to cast to obtain such a dialer.
Also, the code will use the DialContext if passed a dialer that
implements DialContext.
Write a test that proves my point.
Part of https://github.com/ooni/probe/issues/1591.
We already configure a timeout in the underlying dialer, hence
there's no point in keeping the TimeoutDialer around.
Part of https://github.com/ooni/probe/issues/1507
We're currently use jafar for QA and jafar is a better mechanism,
even though it is not portable outside of Linux.
This self censorship mechanism was less cool and added a bunch
of (also cognitive) complexity to netx.
If we ever want to go down a self censorship like road, we probably
want to do as little work as possible in the problem and as much
work as possible inside a helper like jafar.
Part of https://github.com/ooni/probe/issues/1591.
* cleanup(platform): we don't need CGO anymore
Since go1.16, we have the `ios` port. So we can easily
disambiguate between ios and darwin.
This means we don't need to rely on CGO to correctly guess
whether we are on ios or darwin anymore.
So, now miniooni does not depend on a C compiler even
when you are not cross compiling.
* Update internal/platform/platform.go
* refactor(atomicx): move outside the engine package
After merging probe-engine into probe-cli, my impression is that we have
too much unnecessary nesting of packages in this repository.
The idea of this commit and of a bunch of following commits will instead
be to reduce the nesting and simplify the structure.
While there, improve the documentation.
* fix: always use the atomicx package
For consistency, never use sync/atomic and always use ./internal/atomicx
so we can just grep and make sure we're not risking to crash if we make
a subtle mistake on a 32 bit platform.
While there, mention in the contributing guidelines that we want to
always prefer the ./internal/atomicx package over sync/atomic.
* fix(atomicx): remove unnecessary constructor
We don't need a constructor here. The default constructed `&Int64{}`
instance is already usable and the constructor does not add anything to
what we are doing, rather it just creates extra confusion.
* cleanup(atomicx): we are not using Float64
Because atomicx.Float64 is unused, we can safely zap it.
* cleanup(atomicx): simplify impl and improve tests
We can simplify the implementation by using defer and by letting
the Load() method call Add(0).
We can improve tests by making many goroutines updated the
atomic int64 value concurrently.
* refactor(fsx): can live in the ./internal pkg
Let us reduce the amount of nesting. While there, ensure that the
package only exports the bare minimum, and improve the documentation
of the tests, to ease reading the code.
* refactor: move runtimex to ./internal
* refactor: move shellx into the ./internal package
While there, remove unnecessary dependency between packages.
While there, specify in the contributing guidelines that
one should use x/sys/execabs instead of os/exec.
* refactor: move ooapi into the ./internal pkg
* refactor(humanize): move to ./internal and better docs
* refactor: move platform to ./internal
* refactor(randx): move to ./internal
* refactor(multierror): move into the ./internal pkg
* refactor(kvstore): all kvstores in ./internal
Rather than having part of the kvstore inside ./internal/engine/kvstore
and part in ./internal/engine/kvstore.go, let us put every piece of code
that is kvstore related into the ./internal/kvstore package.
* fix(kvstore): always return ErrNoSuchKey on Get() error
It should help to use the kvstore everywhere removing all the
copies that are lingering around the tree.
* sessionresolver: make KVStore mandatory
Simplifies implementation. While there, use the ./internal/kvstore
package rather than having our private implementation.
* fix(ooapi): use the ./internal/kvstore package
* fix(platform): better documentation
* fix(tunnel): pass /absolute/path/to/tor to cretz/bine
It seems cretz/bine is not aware of https://blog.golang.org/path-security
for now. I am planning to send over a diff for that later today.
In the meanwhile, do the right thing here, and make sure that we obtain
the absolute path to the tor binary before we continue.
This work is part of https://github.com/ooni/probe-engine/issues/283.
* fix tests when tor is not installed
* fix(make): correctly write oonimkall.podspec
Part of https://github.com/ooni/probe/issues/1439
* chore: set version number to v3.10.0-beta.1
* fix(ios): don't build a target that requires git
The use cases for using a proxy become more clear over time. When I
originally wrote the proxy code the idea was to use the proxy to proxy
both the communication with the backend and measurements.
It become increasingly clear that we _only_ want to proxy the
communication with the backends. Therefore, there's no point in
skipping the resolver lookup step when we use a proxy.
Part of https://github.com/ooni/probe/issues/985
This fixes an issue where URLs provided with --input are not
accepted by the preventMistakes filter.
The filter itself needs to execute _only_ on URLs returned
by the checkIn API, rather than on URLs returned by the
InputLoader, which may instead be user provided.
Reference issue: https://github.com/ooni/probe/issues/1435
* fix(tunnel/tor): keep tunneldir clean
This diff ensures that we don't keep the log file growing and
we also remove the temporary files created by the library we
are currently using for running tor from golang.
Part of https://github.com/ooni/probe/issues/985
* fix(session.go): tell use we're using a tunnel
* urlgetter: fix tunnel test
This diff fixes the urlgetter test suite to make sure we
are correctly testing for tunnel creation.
While there, improve the way in which we create a testing
directory and add a test for that.
Part of https://github.com/ooni/probe/issues/985.
* fix comment
* fix comment
This functionality should be helpful to test that the general
interface of the tunnel package is okay from the engine package.
Part of https://github.com/ooni/probe/issues/985
* refactor(tunnel): remove nil tunnels hack
This code was originally introduced because a tunnel could be
nil in session.go. I have verified that every invocation of
tunnel.Start is careful to ensure that we have a tunnel name
and that we don't manipulate a nil tunnel.
For this reason, I'd rather remove this tricky bit of code and
further simplify the tunnel code.
Part of https://github.com/ooni/probe/issues/985
* even better docs
* feat: create tunnel inside NewSession
We want to create the tunnel when we create the session. This change
allows us to nicely ignore the problem of creating a tunnel when we
already have a proxy, as well as the problem of locking. Everything is
happening, in fact, inside of the NewSession factory.
Modify miniooni such that --tunnel is just syntactic sugar for
--proxy, at least for now. We want, in the future, to teach the
tunnel to possibly use a socks5 proxy.
Because starting a tunnel is a slow operation, we need a context in
NewSession. This causes a bunch of places to change. Not really a big
deal except we need to propagate the changes.
Make sure that the mobile code can create a new session using a
proxy for all the APIs we support.
Make sure all tests are still green and we don't loose coverage of
the various ways in which this code could be used.
This change is part of https://github.com/ooni/probe/issues/985.
* changes after merge
* fix: only keep tests that can hopefully work
While there, identify other places where we should add more
tests or fix integration tests.
Part of https://github.com/ooni/probe/issues/985
This diff breaks the circular dependency between session and
tunnel, by introducing the concept of early session.
An early session is a session that is able to fetch the psiphon
configuration file _only_ if it's embedded in the binary.
This breaks `miniooni --tunnel=psiphon` for users who have
access to the OONI backend. They are not the users we are
writing this feature for, though, so I think this is reasonable.
At the same time, this opens up the possibility of creating
a psiphon tunnel when constructing a session, which is the
approach I was following in https://github.com/ooni/probe-cli/pull/286.
This work is part of https://github.com/ooni/probe/issues/985.
Once this diff is in, I can land https://github.com/ooni/probe-cli/pull/286.
* feat(tunnel): introduce persistent tunnel state dir
This diff introduces a persistent state directory for tunnels, so that
we can bootstrap them more quickly after the first time.
Part of https://github.com/ooni/probe/issues/985
* fix: make tunnel dir optional
We have many tests where it does not make sense to explicitly
provide a tunnel dir because we're not using tunnels.
This should simplify setting up a session.
* fix(tunnel): repair tests
* final changes
* more cleanups
We're trying to remove a circular dependency between the measurement
Session and the tunnel package. To this end, continue to reduce the
dependency scope by providing TorArgs and TorBinary directly.
Part of https://github.com/ooni/probe/issues/985
We use an optional build tag to hide this configuration. When you
choose this configuration, you need to provide the encrypted config
as well as the corresponding decryption key.
This is not the final design. This is an interim design to start
working and experimenting with this functionality. The general
idea here is to support psiphon in the binaries we build without
committing the psiphon config to the repository itself.
Part of https://github.com/ooni/probe/issues/985
* ongoing
* while there, make sure we test everything
* reorganize previous commit
* ensure we have reasonable coverage in session
The code in here would be better with unit tests. We have too many
integration tests and the tests overall are too slow. But it's also
true that I should not write a giant diff as part of this PR.
* chore(oohelper): increase tests verbosity
Hopefully this helps with https://github.com/ooni/probe/issues/1409.
* fix(oohelper): use a nonstandard resolver
* fix previous
* make the diff pleasant/committable/correct
* chore: update the user-agent we use
Part of the check-list at https://github.com/ooni/probe/issues/1369.
* chore: set version to 3.9.0
See https://github.com/ooni/probe/issues/1369
* chore: run go generate ./...
This is meant to update the bundled CA. We have heard of issues with
our bundled CA, but it seems there have been no changes upstream.
The website https://curl.se/docs/caextract.html still lists as the
last change the one done on Jan 19, 2021, which is the version of
the CA that we're currently bundling.
For the sake of continuing with the release process, I am going
to further investigate the CA once the release is done.
This chore is part of https://github.com/ooni/probe/issues/1369.
* fix(pkg.go.dev): import a subpackage containing the assets
We're trying to fix this issue that pkg.go.dev does not build.
Thanks to @hellais for this very neat idea! Let's keep our
fingers crossed and see whether it fixes!
* feat: use embedded geoip databases
Closes https://github.com/ooni/probe/issues/1372.
Work done as part of https://github.com/ooni/probe/issues/1369.
* fix(assetsx): add tests
* feat: simplify and just vendor uncompressed DBs
* remove tests that seems not necessary anymore
* fix: run go mod tidy
* Address https://github.com/ooni/probe-cli/pull/260/files#r605181364
* rewrite a test in a better way
* fix: gently cleanup the legacy assetsdir
Do not remove the whole directory with brute force. Just zap the
files whose name we know. Then attempt to delete the legacy directory
as well. If not empty, just fail. This is fine because it means the
user has stored other files inside the directory.
* fix: create .miniooni if missing
* fetch RiseupVPN CA cert with MultiGetter. It allows us to write better tests and ensures this test step is added in the logs
* Implement TransportStatus for RiseupVPN tests. It indicates if a whole transport is blocked, which is considered as a test anomaly
* Redesign unit tests for RiseupVPN. Instead of a real backend, mocked server responses are used. Tests for invalid CA certs and for TransportStatus are added.
* Update internal/engine/experiment/riseupvpn/riseupvpn.go
Co-authored-by: Simone Basso <bassosimone@gmail.com>
