This diff implements the first two cleanups defined at https://github.com/ooni/probe/issues/1956:
> - [ ] observe that `netxlite` and `netx` differ in error wrapping only in the way in which we set `ErrWrapper.Operation`. Observe that the code using `netxlite` does not care about such a field. Therefore, we can modify `netxlite` to set such a field using the code of `netx` and we can remove `netx` specific code for errors (which currently lives inside of the `./internal/engine/legacy/errorsx` package
>
> - [ ] after we've done the previous cleanup, we can make all the classifiers code private, since there's no code outside `netxlite` that needs them
A subsequent diff will address the remaining cleanup.
While there, notice that there are failing, unrelated obfs4 tests, so disable them in short mode. (I am confident these tests are unrelated because they fail for me when running test locally from the `master` branch.)
Since https://github.com/ooni/probe-cli/pull/527, if an experiment
returns an error, the corresponding measurement is not submitted since
the semantics of returning an error is that something fundamental
went wrong (e.g., we could not parse the input URL).
This diff ensures that all experiments only return and error when
something fundamental was wrong and return nil otherwise.
Reference issue: https://github.com/ooni/probe/issues/1808.
This commit completely removes the original netx implementation,
which was only used by `tor`, since this has changed in
https://github.com/ooni/probe-cli/pull/652.
The original netx implementation was my first attempt at performing
network measurements using Go. It started its life inside of the
https://github.com/ooni/netx repository. It was later merged into
the https://github.com/ooni/probe-engine repository. It finally
ended up into this repository when we merged probe-engine with it.
The main issue with the original implementation is that it was
a bit too complex and used channels where they were probably not
necessary. Because of that, later I introduced a second netx
implementation, which currently lives in ./internal/engine/netx.
The current netx implementation, the third one, lives in the
./internal/netxlite package. We are currently working to replace
the second implementation with the third one, but this is happening
at a slow pace. Also, the second implementation does not have big
maintenance concerns but it's just a bit too bureaucratic to use
since it involves creating lots of `Config` structures.
The reference issue is probably https://github.com/ooni/probe/issues/1688,
since this diff has been enabled by rewriting Tor to use `measurex`
(a library living on top of `netxlite`).
This diff rewrites the tor experiment to use measurex "easy" API.
To this end, we need to introduce an "easy" measurex API, which basically
performs easy measurements returning two pieces of data:
1. the resulting measurement, which is already using the OONI
archival data format and is always non-nil
2. a failure (i.e., the pointer to an error string), which
is nil on success and points to a string on failure
With this change, we should now be able to completely dispose of
the original netx API, which was only used by tor.
Reference issue: https://github.com/ooni/probe/issues/1688.
1. we want optionally to log the body (we don't want to log the body
when we're fetching psiphon secrets or tor targets)
2. we want body logging to _also_ happen on error since this is quite
useful to debug possible errors when accessing the API
This diff adds the above functionality, which were previously
described in https://github.com/ooni/probe/issues/1951.
This diff also adds comprehensive testing.
* refactor(httpx): use mocks to implement tests
While there, make sure no test depends on external services by
replacing such tests with httptest.
See https://github.com/ooni/probe/issues/1951.
* fix(httpx): ensure we honour the context
This diff extracts the fakefiller inside of internal/ooapi (a
currently unused package) into its own package.
The fakefiller knows how to fill many fields that are typically
shared as data structures across processes.
It is not perfect in that it cannot fill logger or http client
fields, but still helps with better filling and testing.
So, here we're using the fakefiller to improve testing of httpx
and, nicely enough, we've already catched a bug in the way in
which APIClientTemplate.Build misses to forward Authorization from
the original template. Yay!
Work part of https://github.com/ooni/probe/issues/1951
As mentioned in https://github.com/ooni/probe/issues/1951, one of
the main issues I did see with httpx.APIClient is that in some cases
it's used in a very fragile way by probeservices.Client.
This happens in psiphon.go and tor.go, where we create a copy of
the APIClient and then modify it's Authorization field.
If we ever refactor probeservices.Client to take a pointer to
httpx.Client, we are now mutating the httpx.Client.
Of course, we don't want that to happen.
This diff attempts to address such a problem as follows:
1. we create a new APIClientTemplate type that holds the same
fields of an APIClient and allows to build an APIClient
2. we modify every user of APIClient to use APIClientTemplate
3. when we need an APIClient, we build it from the corresponding
template and, when we need to use a specific Authorization, we
use a build factory that sets APIClient.Authorization
4. we hide APIClient by renaming it apiClient and by defining
an interface called APIClient that allows to use it
So, now the codebase always uses the opaque APIClient interface to
issue API calls and always uses the APIClientTemplate to build an
opaque APIClient.
Boom! We have separated construction from usage and we are not
mutating in weird ways the APIClient anymore.
This PR starts to implement the refactoring described at https://github.com/ooni/probe/issues/1951. I originally wrote more patches than the ones in this PR, but overall they were not readable. Since I want to squash and merge, here's a reasonable subset of the original patches that will still be readable and understandable in the future.
This diff lightly refactors the code in measurex to allow a user
to configure all possible timeouts and the max-snapshot-size.
There is currently a little bit of tension between setting timeouts
inside of measurex and the watchdog timeouts inside of netxlite.
This tension has been documented.
Let us repeat the issue also in this commit message. If you are
using a masurex.Measurer configured with very large timeouts and
the underlying netxlite implementation uses shorter whatchdog
timeouts, then you are going to see shorter than expected timeouts.
Ideally, we would like to have just a single timeout but there is
no way to ask the context "hey, can you tell me if you already have
a configured timeout?".
It may be that the right solution is to modify netxlite to have
some sort of root/library object with this configuration.
If that's the case, then a Measurer could be refactored as follows:
- create the underlying netxlite "library"
- initialize the timeouts desired by the Measurer
- create a Dialer, of whatever is needed
- use it
Now this is not possible because netxlite timeouts are internal
static settings rather than attributes of a structure.
Anyway, for now I'm happy with this just being documented.
(I suspect this issue will need to be addresses when we'll write
unit tests for measurex; at that time a proper solution should
come out naturally due to the unit tests constraints.)
I'm working on this refactoring, BTW, to facilitate rewriting `tor`
using measurex (see https://github.com/ooni/probe/issues/1688).
## Checklist
- [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md)
- [x] reference issue for this pull request: https://github.com/ooni/probe/issues/1885
- [x] related ooni/spec pull request: N/A
Location of the issue tracker: https://github.com/ooni/probe
## Description
This PR contains a set of changes to move important interfaces and data types into the `./internal/model` package.
The criteria for including an interface or data type in here is roughly that the type should be important and used by several packages. We are especially interested to move more interfaces here to increase modularity.
An additional side effect is that, by reading this package, one should be able to understand more quickly how different parts of the codebase interact with each other.
This is what I want to move in `internal/model`:
- [x] most important interfaces from `internal/netxlite`
- [x] everything that was previously part of `internal/engine/model`
- [x] mocks from `internal/netxlite/mocks` should also be moved in here as a subpackage
This branch adds support for running:
1. `go-libtor` on mobile.
2. the tor provided by the desktop app via the `OONI_TOR_BINARY` environment variable.
See https://github.com/ooni/ooni.org/issues/761.
Co-authored-by: Simone Basso <bassosimone@gmail.com>
This commit introduces a new `InputLoader` policy by which, if no
input is provided, we use a static default input list.
We also modify the code to use this policy for dnscheck and
stunreachability, with proper input.
We also modify `miniooni` to pass the new `ExperimentName` field to
the `InputLoader` to indicate which default input list to use.
This diff is part of a set of diffs aiming at fixing
https://github.com/ooni/probe/issues/1814 and has been
extracted from https://github.com/ooni/probe-cli/pull/539.
What remains to be done, after this diff has landed is to ensure
things also work for ooniprobe and oonimkall.
We want stunreachability to use the same STUN servers used by
snowflake, so let's start by making a common package holding the
servers. Let's also use this new package in Snowflake.
We're currently not using this package in stunreachability, but
I am going to apply this as a subsequent diff.
Reference issue: https://github.com/ooni/probe/issues/1814. This
issue is a bit complex to address in a single PR, so we are going
to proceed incremntally.
This diff was extracted from https://github.com/ooni/probe-cli/pull/539.
Here we're refactoring stunreachability to not provide internally a
default input and to take in input an URL rather than a string.
The related ooni/spec change is https://github.com/ooni/spec/pull/227.
This diff has been extracted from https://github.com/ooni/probe-cli/pull/539.
Because the original diff was large, I'm splitting it in a set of
more easily manageable diffs.
The reference issue is https://github.com/ooni/probe/issues/1814, which
is complex enough to require us to proceed incrementally.
This diff WILL need to be backported to release/3.11.
* [forwardport] fix(oonimkall): make logger used by tasks unit testable (#623)
This diff forward ports e4b04642c51e7461728b25941624e1b97ef0ec83.
Reference issue: https://github.com/ooni/probe/issues/1903
* [forwardport] feat(oonimkall): improve taskEmitter testability (#624)
This diff forward ports 3e0f01a389c1f4cdd7878ec151aff91870a0bdff.
1. rename eventemitter{,_test}.go => taskemitter{,_test}.go because
the new name is more proper after we merged the internal/task package
inside of the oonimkall package;
2. rename runner.go's `run` function to `runTask`;
3. modify `runTask` to use the new `taskEmitterUsingChan` abstraction
on which we will spend more works in a later point of this list;
4. introduce `runTaskWithEmitter` factory that is called by `runTask`
and allows us to more easily write unit tests;
5. acknowledge that `runner` was not using its `out` field;
6. use the new `taskEmitterWrapper` in `newRunner`;
7. acknowledge that `runnerCallbacks` could use a generic
`taskEmitter` as field type rather than a specific type;
8. rewrite tests to use `runTaskWithEmitter` which leads to
simpler code that does not require a goroutine;
9. acknowledge that the code has been ignoring the `DisabledEvents`
settings for quite some time, so stop supporting it;
10. refactor the `taskEmitter` implementation to be like:
1. we still have the `taskEmitter` interface;
2. `taskEmitterUsingChan` wraps the channel and allows for
emitting events using the channel;
3. `taskEmitterUsingChan` owns an `eof` channel that is
closed by `Close` (which is idempotent) and signals we
should be stop emitting;
4. make sure `runTask` creates a `taskEmitterUsingChan`
and calls its `Close` method when done;
5. completely remove the code for disabling events
since the code was actually ignoring the stting;
6. add a `taskEmitterWrapper` that adds common functions
for emitting events to _any_ `taskWrapper`;
7. write unit tests for `taskEmitterUsingChan` and
for `taskEmitterWrapper`;
11. acknowledge that the abstraction we need for testing is
actually a thread-safe thing that collects events into a
vector containing events and refactor all tests accordingly.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): make the runner unit-testable (#625)
This diff forward ports 9423947faf6980d92d2fe67efe3829e8fef76586.
See https://github.com/ooni/probe/issues/1903
* [forwardport] feat(oonimkall): write unit tests for the runner component (#626)
This diff forward ports 35dd0e3788b8fa99c541452bbb5e0ae4871239e1.
Forward porting note: compared to 35dd0e3788b8fa99c541452bbb5e0ae4871239e1,
the diff I'm committing here is slightly different. In `master` we do not
have the case where a measurement fails and a measurement is returned, thus
I needed to adapt the test to become like this:
```diff
diff --git a/pkg/oonimkall/runner_internal_test.go b/pkg/oonimkall/runner_internal_test.go
index 334b574..84c7436 100644
--- a/pkg/oonimkall/runner_internal_test.go
+++ b/pkg/oonimkall/runner_internal_test.go
@@ -568,15 +568,6 @@ func TestTaskRunnerRun(t *testing.T) {
}, {
Key: failureMeasurement,
Count: 1,
- }, {
- Key: measurement,
- Count: 1,
- }, {
- Key: statusMeasurementSubmission,
- Count: 1,
- }, {
- Key: statusMeasurementDone,
- Count: 1,
}, {
Key: statusEnd,
Count: 1,
```
I still need to write more assertions for each emitted event
but the code we've here is already a great starting point.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): merge files, use proper names, zap unneeded integration tests (#627)
This diff forward ports f894427d24edc9a03fc78306d0093e7b51c46c25.
Forward porting note: this diff is slightly different from the original
mentioned above because it carries forward changes mentioned in the
previous diff caused by a different way of handling a failed measurement
in the master branch compared to the release/3.11 branch.
Move everything that looked like "task's model" inside of the
taskmodel.go file, for consistency.
Make sure it's clear some variables are event types.
Rename the concrete `runner` as `runnerForTask`.
Also, remove now-unnecessary (and flaky!) integration tests
for the `runnerForTask` type.
While there, notice there were wrong URLs that were generated
during the probe-engine => probe-cli move and fix them.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): we can simplify StartTask tests (#628)
This diff forward ports dcf2986c2032d8185d58d24130a7f2c2d61ef2fb.
* refactor(oonimkall): we can simplify StartTask tests
We have enough checks for runnerForTask. So we do not need to
duplicate them when checking for StartTask.
While there, refactor how we start tasks to remove the need for
extra runner functions.
This is the objective I wanted to achieve for oonimkall:
1. less duplicate tests, and
2. more unit tests (which are less flaky)
At this point, we're basically done (pending forwardporting to
master) with https://github.com/ooni/probe/issues/1903.
* fix(oonimkall): TestStartTaskGood shouldn't cancel the test
This creates a race condition where the test may fail if we cannot
complete the whole "Example" test in less than one second.
This should explain the build failures I've seen so far and why
I didn't see those failures when running locally.
* [forwardport] fix(webconnectivity): send specific user agent (#615)
This forward ports b8c530388e66b2cc86abad26d077202782e4a823 to `master`.
See https://github.com/ooni/probe/issues/1902
* fix(websteps): send the correct user agent
Also related to https://github.com/ooni/probe/issues/1902: let's just
ensure that also websteps behaves in the correct way.
This diff forwardports 856e436e20d511a4f0d618546da7921fa9f8c5f6 to the master branch
Original commit message:
- - -
This pull request changes `mk` and github workflows to build and publish binaries on tag. We also update the documentation to explain this new branching model. Basically, we have release branches where we produce binary packages and we add extra code, on tag, to publish such packages inside a release.
We discussed removing most secrets from builds in this repository and having a different tool/repository that takes in input also secrets for doing follow-up actions after publishing. As a consequence, this pull request also removes all pieces of code that require secrets. The next step is to reinstate this code in this new repository/tool.
The existing code in `mk` also implemented caching. This feature was useful when doing local builds because it reduced the time required to obtain binary releases. With builds running as part of GitHub actions, we don't need caching because we spawn parallel machines to build binaries. Therefore, let us also remove caching, which makes the code simpler. (Caching in itself is hard and in https://github.com/ooni/probe/issues/1875 I noted that, for example, caching of the `ooni/go` repository was leading to some unwanted behaviour when changing the branch. Without caching, this behaviour is gone and we always generally use fresh information to produce builds.) Of course, this means that local builds are now slower, but I do not think this is a problem _because_ we want to use GitHub actions for building in the common case.
Reference issues: https://github.com/ooni/probe/issues/1879 and https://github.com/ooni/probe/issues/1875.
The final aspect to mention to conclude this description is an implementation one:
```
gh release create -p $tag --target $GITHUB_SHA || true
```
The code above uses `|| true` because there could already be a release. So, basically, it means that, if a release does not already exist, then we're going to create one. Otherwise, it does not matter because there's already a release.
This diff forward ports ea44e99451f345474738b9010ff791759a1f1367.
Original commit message:
- - -
This change allows for producing cloud builds using the psiphon
config files. We will add those files as build secrets. Only people
in the organization and collaborators with at least "write"
access could trigger builds containing such secrets.
Before this change, `./mk` unconditionally attempted to clone
github.com/ooni/probe-private. Now, it only checks whether
we need to clone _if_ files are not already there.
This allows us to use GitHub actions and secrets to copy the
files in there _without_ needing to clone a private repo.
Cloning a private repo would require us to include as repository
secret an access token with full `repo` scope, which is a very
broad scope. Instead, by using secrets to include psiphon config,
we are narrowing down the secrets required to make a release build.
See https://github.com/ooni/probe/issues/1878
This diff WILL require forward porting to the master branch.
This diff forward ports f47b0c6c16e0cd417e3591358eb85b45962f307d to master.
Original commit message:
- - -
1. we now need to name the framework `.xcframework` otherwise
gomobile refuses to build a new framework for us ¯\_(ツ)_/¯
2. remove duplicate errno definition for iOS (iOS and darwin
are considered the same, therefore we don't need iOS defs)
Reference issue for this PR: https://github.com/ooni/probe/issues/1876
This diff WILL need to be forwardported to master.
This commit forward ports dedd84fa7ecb09f718f6b1a9c83999cb37b34dfa.
Original commit message:
- - -
This diff changes code the release/3.11 branch to ensure we're not using dns.google and www.google.com over HTTP3. As documented in https://github.com/ooni/probe/issues/1873, since this morning (approx) these services do not support HTTP3 anymore. (I didn't bother with checking whether this issue affects _other_ Google services; I just limited my analysis to the services that we were using as part of testing.)
This patch WILL require forward porting to the master branch.
This commit forward ports 74947dbbd12266c12a38fad51a70fc78a21720fd from
the `release/3.11` branch to `master`. Here's the original commit message:
- - -
Android is also Linux. The Android build fails because both
errno_linux.go and errno_android.go are compiled.
There's no difference between the files except into a comment
that mentions "linux" or "android".
Therefore, it's safe to remove the android-specific file
and just keep and use the linux-specific one.
Part of https://github.com/ooni/probe/issues/1863, where we're
forward porting ooni/go patches to go1.17.
I'm still trying to figure out whether I can build oonimkall
using the forward ported patches and this error prevents me
from building, because the build fails.
"やれやれだぜ"
Note that this patch WILL need to be forward ported to master.
This bug was previosuly reported to me by @hellais.
Because I did run `go generate ./internal/netxlite/...` we also
get for free updated certificates, which is OK.
1. introduce implementations of HTTPTransport and HTTPClient
that apply an error wrapping policy using the constructor
for a generic top-level error wrapper
2. make sure we use the implementations in point 1 when we
are constructing HTTPTransport and HTTPClient
3. make sure we apply error wrapping using the constructor for
a generic top-level error wrapper when reading bodies
4. acknowledge that error wrapping would be broken if we do
not return the same classification _and_ operation when we wrap
an already wrapped error, so fix the to code to do that
5. acknowledge that the classifiers already deal with preserving
the error string and explain why this is a quirk and why we
cannot remove it right now and what needs to happen to safely
remove this quirk from the codebase
Closes https://github.com/ooni/probe/issues/1860
Each tutorial provides you a piece of knowledge. Add some text in
the main readme file explaining the big picture to the reader.
Closes https://github.com/ooni/probe/issues/1819
This diff changes the algorithm used by webconnectivity's
httpanalysis.go to ignore any status code <= 0 rather
than just ignoring the == 0 case.
Make sure we add test cases for when the control's status
code is negative rather than being zero.
While there, simplify code where boolean checks could be
more compact according to staticcheck.
Closes https://github.com/ooni/probe/issues/1825
This change should simplify the pipeline's job.
Reference issue: https://github.com/ooni/probe/issues/1817.
I previously dismissed this possibility, but now it seems clear it
is simpler to have a very tabular data format internally and to
convert such a format to OONI's data format when serializing.
The OONI data format is what the pipeline expects, but processing
is easier with a more linear/tabular format.
This reverts commit 851b9913fa because
it seems it's not enough to allow us to see certificate errors with
quic, plus it's complex code. So, we'd rather develop a better approach,
and perhaps a simpler one, that works with QUIC as well.
This is the policy we need to provoke certificate errors. We'll divert
from, say, `8.8.8.8:443/udp` to, say, `1.1.1.1:443/udp`.
We'll do something similar for `443/tcp`.
This will cause certificate validation errors.
With this change, we have now implemented the simple design described
by https://github.com/ooni/probe/issues/1803#issuecomment-957323297.
When we're testing multiple endpoints, it's quite important to control
the order with which they are returned to the code.
This feature is especially relevant to Web Connectivity, which will
check the endpoints to connect to in order.
Therefore, we need to force deterministic results to ensure that we can
have deterministic tests when doing Web Connectivity QA.
This diff gives us the guarantee that we can have determinism.
Part of https://github.com/ooni/probe/issues/1803#issuecomment-957323297.
1. in normal code is better to always do if err != nil so that
the ifs only contain error code (this is ~coding policy)
2. in tests we want to ensure we narrow down the error to the
real error that happened, to have greater confidence
Written while working on https://github.com/ooni/probe/issues/1803#issuecomment-957323297
This change will simplify follow-up work done as part of
https://github.com/ooni/probe/issues/1803#issuecomment-957323297 to
implement a comprehensive self-censoring solution.
While there, rename the "proxy" action to "pass" because what we
are effectively doing is passing traffic to the network (that's a
minor change but it seems a better analogy).
Reducing the errors is not done in a perfect way.
We have documented the most striking differences inside
https://github.com/ooni/probe/issues/1707#issuecomment-942283746 and
some attempts to improve the situation further inside
https://github.com/ooni/probe/issues/1707#issuecomment-942341255.
A better strategy for the future would be to introduce more
specific timeout errors, such as dns_timeout_error, etc.
More testing may be needed to further validate and compare the
old and the new TH, but this requires Jafar improvements to
more precisely simulate more complex censorship.
This is the most immediate fix to the issue described by
https://github.com/ooni/probe/issues/1792.
So, the logic was actually miss the increment, which
would have been noticed with proper unit testing.
Anyway, I am not sure why the loop ensues in the first
time. By looking at the headers, it seems we're passing
the headers correctly.
So, even though this fix interrupts the loop, it still
remains the question of whether the loop is legit or
whether we're missing extra logic to properly redirect.
This diff adds the prototype websteps implementation that used
to live at https://github.com/ooni/probe-cli/pull/506.
The code is reasonably good already and it's pointing to a roaming
test helper that I've properly configured.
You can run websteps with:
```
./miniooni -n websteps
```
This will go over the test list for your country.
At this stage the mechanics of the experiment is set, but we
still need to have a conversation on the following topics:
1. whether we're okay with reusing the data format used by other
OONI experiments, or we would like to use a more compact data
format (which may either be a more compact JSON or we can choose
to always submit compressed measurements for websteps);
2. the extent to which we would like to keep the measurement as
a collection of "the experiment saw this" and "the test helper
saw that" and let the pipeline choose an overall score: this is
clearly an option, but there is also the opposite option to
build a summary of the measurement on the probe.
Compared to the previous prototype of websteps, the main
architectural change we have here is that we are following
the point of view of the probe and the test helper is
much more dumb. Basically, the probe will choose which
redirection to follow and ask the test helper every time
it discovers a new URL to measure it w/o redirections.
Reference issue: https://github.com/ooni/probe/issues/1733
This commit introduce a measurement library that consists of
refactored code from earlier websteps experiments.
I am not going to add tests for the time being, because this library
is still a bit in flux, as we finalize websteps.
I will soon though commit documentation explaining in detail how
to use it, which currrently is at https://github.com/ooni/probe-cli/pull/506
and adds a new directory to internal/tutorial.
The core idea of this measurement library is to allow two
measurement modes:
1. tracing, which is what we're currently doing now, and the
tutorial shows how we can rewrite the measurement part of web
connectivity with measurex using less code. Under a tracing
approach, we construct a normal http.Client that however has
tracing configured, we gather events for resolve, connect, TLS
handshake, QUIC handshake, HTTP round trip, etc. and then we
try to make sense of what happened from the events stream;
2. step-by-step, which is what websteps does, and basically
means that after each operation you immediately write into
a Measurement structure its results and immediately draw the
conclusions on what seems odd (which later may become an
anomaly if we see what the test helper measured).
This library is also such that it produces a data format
compatible with the current OONI spec.
This work is part of https://github.com/ooni/probe/issues/1733.
This is required to implement websteps, which is currently tracked
by https://github.com/ooni/probe/issues/1733.
We introduce the concept of async runner. An async runner will
post measurements on a channel until it is done. When it is done,
it will close the channel to notify the reader about that.
This change causes sync experiments now to strictly return either
a non-nil measurement or a non-nil error.
While this is a pretty much obvious situation in golang, we had
some parts of the codebase that were not robust to this assumption
and attempted to submit a measurement after the measure call
returned an error.
Luckily, we had enough tests to catch this change in our assumption
and this is why there are extra docs and tests changes.
At the moment ooapi is not used. It will eventually be used since
it's a better way of accessing the OONI backend API.
To fix these tests, we need to fix the swagger emitted by the
backend API, which is not a priority at the moment, since we are
working instead to integrate websteps in miniooni.
Issue https://github.com/ooni/probe/issues/1790 tracks the work
required to re-enabled the tests I'm skipping with this diff.
This work is part of https://github.com/ooni/probe/issues/1733.
* feat: run ~always netxlite integration tests
This diff ensures that we check on windows, linux, macos that our
fundamental networking library (netxlite) works.
We combine unit and integration tests.
This work is part of https://github.com/ooni/probe/issues/1733, where
I want to have more strong guarantees about the foundations.
* fix(filtering/tls_test.go): make portable on Windows
The trick here is to use the wrapped error so to normalize the
different errors messages we see on Windows.
* fix(netxlite/quic_test.go): make portable on windows
Rather than using the zero port, use the `x` port which fails
when the stdlib is parsing the address.
The zero port seems to work on Windows while it does not on Unix.
* fix(serialresolver_test.go): make error more timeout than before
This seems enough to convince Go on Windows about this error
being really a timeout timeouty timeouted thingie.
On Windows, GetAddrInfoW is a syscall and the Go resolver does
not attempt to map errors beyond WSA_HOST_NOT_FOUND, which becomes
"no such host", which we map to "dns_nxdomain_error".
See https://github.com/golang/go/blob/go1.17.1/src/net/lookup_windows.go#L16.
To map more GetAddrInfoW errors, thus, we need to enhance our
error classifier to have system specific errors.
Then, we need to filter for the WSA errors that are most likely
to pop up and map them to OONI failures. Those are three:
- WSANO_DATA which we have from our own UDP resolver as well
and which we can map to `dns_no_answer`
- WSANO_RECOVERY which we don't have but existed for MK so
we will use `dns_non_recoverable_failure`, which was an MK error
- WSATRY_AGAIN which likewise we map to the error that MK
used to emit, so `dns_temporary_failure`
This diff should address https://github.com/ooni/probe/issues/1467.
I need to run test on Windows and I just discovered that:
1. the `errno_unix.go` filename does not mean anything because
`unix` is not a valid platform, so we need a filename for
each platform that we care about;
2. on Windows we need to use WSA prefixed names;
3. `i/e/session_psiphon.go` was not building because of the
migration from `netxlite/iox` to `netxlite`.
This diff attempts to fix all three issues.
The reference issue is https://github.com/ooni/probe/issues/1733,
because I was working on such an issue.
When preparing a tutorial for netxlite, I figured it is easier
to tell people "hey, this is the package you should use for all
low-level networking stuff" rather than introducing people to
a set of packages working together where some piece of functionality
is here and some other piece is there.
Part of https://github.com/ooni/probe/issues/1591
I have recently seen a data race related our way of
mutating the outgoing request to set the host header.
Unfortunately, I've lost track of the race output,
because I rebooted my Linux box before saving it.
Though, after inspecting why and and where we're mutating
outgoing requets, I've found that:
1. we add the host header when logging to have it logged,
which is not a big deal since we already emit the URL
rather than just the URL path when logging a request, and
so we can safely zap this piece of code;
2. as a result, in measurements we may omit the host header
but again this is pretty much obvious from the URL itself
and so it should not be very important (nonetheless,
avoid surprises and keep the existing behavior);
3. when the User-Agent header is not set, we default to
a `miniooni/0.1.0-dev` user agent, which is probably not
very useful anyway, so we can actually remove it.
Part of https://github.com/ooni/probe/issues/1733 (this diff
has been extracted from https://github.com/ooni/probe-cli/pull/506).
While there, modernize the way in which we run tests to avoid
depending on the fake files scattered around the tree and to
use some well defined mock structures instead.
Part of https://github.com/ooni/probe/issues/1591
There are a bunch of packages where we don't really need to depend
on netx but we can use local definitions that describe what we are
expecting from data structures we receive in input. This diff
addresses one of such cases.
Part of https://github.com/ooni/probe/issues/1591
I discovered which transport were used by apitool and made sure he gets the same transports now. While there, I discovered an issue with ooni/oohttp that has been fixed with cba9b1ce5e.
Part of https://github.com/ooni/probe/issues/1591
* netxlite: improve docs, tests, and code quality
* better documentation
* more strict testing of dialer (especially make sure we
document the quirk in https://github.com/ooni/probe/issues/1779
and we have tests to guarantee we don't screw up here)
* introduce NewErrWrapper factory for creating errors so we
have confidence we are creating them correctly
Part of https://github.com/ooni/probe/issues/1591
Adapt other places where it was not using a logger to either choose
a reasonable logger or disable logging for backwards compat.
See https://github.com/ooni/probe/issues/1591
This simplifies serializing errors to `*string`. It did not
occur to me before. It seems quite a nice improvement.
Part of https://github.com/ooni/probe/issues/1591
They are now more readable. I'll do another pass and start
separating integration testing from unit testing.
I think we need to have some always on integration testing
for netxlite that runs on macOS, linux, and windows.
See https://github.com/ooni/probe/issues/1591
No real functional change. A few are needed and they will come
next. With this diff I just wanted to do cosmetic changes and
documentation changes, to ensure this package is okay.
See https://github.com/ooni/probe/issues/1591
We need still to add similar wrappers to internal/netxlite but we
will adopt a saner approach to error wrapping this time.
See https://github.com/ooni/probe/issues/1591
For consistency and also because the SafeErrorWrapperBuilder seems
to be the building pattern than the original code assumed.
New code should not use it, but I'd rather keep legacy code consistent
formally and with its own original assumptions.
In particular, it matters that SafeErrorWrapperBuilder assigns the
most relevant operation that failed. We were not doing that when we
were manually creating a new ErrWrapper.
Part of https://github.com/ooni/probe/issues/1591
The legacy part for now is internal/errorsx. It will stay there until
I figure out whether it also needs some extra bug fixing.
The good part is now in internal/netxlite/errorsx and contains all the
logic for mapping errors. We need to further improve upon this logic
by writing more thorough integration tests for QUIC.
We also need to copy the various dialer, conn, etc adapters that set
errors. We will put them inside netxlite and we will generate errors in
a way that is less crazy with respect to the major operation. (The
idea is to always wrap, given that now we measure in an incremental way
and we don't measure every operation together.)
Part of https://github.com/ooni/probe/issues/1591
We will move the sane part of this package to i/netxlite/errorsx
and we will move the rest to i/e/legacy/errorsx.
What is the sane part? The sane part is error classifiers plus
the definition of ErrWrapper. The rest, including the rules
on how to decide whether an operation is major, are tricky and
we should consider them legacy and replace them with rules
that are more easy to understand and reason on.
Part of https://github.com/ooni/probe/issues/1591
No functional change, as it's clearly obvious from the output.
While there, also rename the generator for certifi. We are planning
on merging errorsx into netxlite. The first step is to give different
names to the code generating programs.
See https://github.com/ooni/probe/issues/1591
This is the last bit of functionality we need before rewriting a
chunk of websteps to use netxlite.
To rewrite most of it, we still need to move over:
1. dnstransport code
2. errorsx code
With both done, netxlite is a good library for websteps as well
as for most other operations we perform outside of the experiments.
Part of https://github.com/ooni/probe/issues/1591
With this change, we are now able to change more dependent code to simplify
the way in which we create and manage resolvers.
See https://github.com/ooni/probe/issues/1591
Like before, do not touch the rest of the tree. Rather create
compatibility types declared as legacy.
We will soon be able to close idle connections for an HTTP3
transport using any kind of resolvers more easily.
See https://github.com/ooni/probe/issues/1591
This quirk really saddens me. It's a piece of tech debt we're
carrying over from the original netx implementation.
We cannot remove it _until_ we have legacy netx code around.
The second best thing we can do is to clearly move this code in
a place where it's clear it's a quirk and write and use some extra
code that makes sure the quirk's assumptions are always met.
Sigh.
See https://github.com/ooni/probe/issues/1591
To make this happen, we need to take as argument a TLSDialer rather than
a TLSHandshaker. Then, we need to arrange the code so that we always
enforce a timeout for both TCP and TLS connections.
Because a TLSDialer can be constructed with a custom TLSConfig, we cover
also the case where the users wants to provide such a config.
While there, make sure we have better unit tests of the HTTP code.
See https://github.com/ooni/probe/issues/1591
While there reorganize mocks' tls implementation to use a single file
called tls.go (and tls_test.go) just like netxlite does.
While there write tests ensuring we always add timeouts when we are
making TCP connections (be them TLS or cleartext).
See https://github.com/ooni/probe/issues/1591
This basically adapts already existing code inside websteps to
instead be into the netxlite package, where it belongs.
In the process, abstract the TLSDialer but keep a reference to the
previous name to avoid refactoring existing code (just for now).
While there, notice that the right name is CloseIdleConnections (i.e.,
plural not singular) and change the name.
While there, since we abstracted TLSDialer to be an interface, create
suitable factories for making a TLSDialer type from a Dialer and a
TLSHandshaker.
See https://github.com/ooni/probe/issues/1591
We are proceeding with this plan of every major type being able to
close idle connections, which will simplify making DNS resolvers.
See https://github.com/ooni/probe/issues/1591.
Like we did before for the resolver, a dialer should propagate the
request to close idle connections to underlying types.
See https://github.com/ooni/probe/issues/1591
* fix(netxlite): make default resolver converge faster
Closes https://github.com/ooni/probe/issues/1726
* Update internal/netxlite/resolver.go
* fix(ndt7): adapt tests after previous change
Because now we're running the DNS resolution inside a goroutine
with a child context, the returned error string is different.
The previous error said we canceled the whole dialing operation,
while now we see directly that the context was canceled.
We would like to refactor the code so that a DoH resolver owns the
connections of its underlying HTTP client.
To do that, we need first to incorporate CloseIdleConnections
into the Resolver model. Then, we need to add the same function
to all netxlite types that wrap a Resolver type.
At the same time, we want the rest of the code for now to continue
with the simpler definition of a Resolver, now called ResolverLegacy.
We will eventually propagate this change to the rest of the tree
and simplify the way in which we manage Resolvers.
To make this possible, we introduce a new factory function that
adapts a ResolverLegacy to become a Resolver.
See https://github.com/ooni/probe/issues/1591.
## Description
This PR continues the refactoring of `netx` under the following principles:
1. do not break the rest of the tree and do not engage in extensive tree-wide refactoring yet
2. move under `netxlite` clearly related subpackages (e.g., `iox`, `netxmocks`)
3. move into `internal/netxlite/internal` stuff that is clearly private of `netxlite`
4. hide implementation details in `netxlite` pending new factories
5. refactor `tls` code in `netxlite` to clearly separate `crypto/tls` code from `utls` code
After each commit, I run `go test -short -race ./...` locally. Each individual commit explains what it does. I will squash, but this operation will preserve the original commit titles, so this will give further insight on each step.
## Commits
* refactor: rename netxmocks -> netxlite/mocks
Part of https://github.com/ooni/probe/issues/1591
* refactor: rename quicx -> netxlite/quicx
See https://github.com/ooni/probe/issues/1591
* refactor: rename iox -> netxlite/iox
Regenerate sources and make sure the tests pass.
See https://github.com/ooni/probe/issues/1591.
* refactor(iox): move MockableReader to netxlite/mocks
See https://github.com/ooni/probe/issues/1591
* refactor(netxlite): generator is an implementation detail
See https://github.com/ooni/probe/issues/1591
* refactor(netxlite): separate tls and utls code
See https://github.com/ooni/probe/issues/1591
* refactor(netxlite): hide most types but keep old names as legacy
With this change we avoid breaking the rest of the tree, but we start
hiding some implementation details a bit. Factories will follow.
See https://github.com/ooni/probe/issues/1591
The quic-go library does not support it anymore. So, let us be consistent
and remove any reference to h3-29 from our codebase.
Closes https://github.com/ooni/probe/issues/1740.
* fix: disable debianrepo build on master branch
This just mitigates https://github.com/ooni/probe/issues/1741 and does
not fully address it, but I'd rather avoid delving into this problem until
I open a release/v3.11.0 branch and have to really fix this issue.
* fix: only run coverage using go1.17
This is the version of Go with which we are going to bless v3.11.0
therefore it's the only version of Go that matters.
Reference issue: https://github.com/ooni/probe/issues/1738.
* fix(ptx/obfs4_test.go): avoid context-vs-normal-code race
We want to test whether we get the context failure if the error
generated inside normal code happens _after_ the context cancellation.
The best way to do that is to write code that is not racy. To this
end, we just need to pause normal code until we know that the context
has returned to the caller. We also need to ensure we do not leak
a goroutine, hence we use a WaitGroup to check that.
Fixes https://github.com/ooni/probe/issues/1750
When a probe gets a local DNS failure, it will continue and nonetheless
query the test helper without any IP address, just an empty list.
This diff fixes the behavior of cmd/oohelper to do the same.
Work part of https://github.com/ooni/probe/issues/1707.
Reference issue: https://github.com/ooni/probe/issues/1769
Motivation: The CI is failing. Those are integration tests. Let us figure out the issue when we approach release. Until we approach release, do not let those tests distracting us. Normal merges should only pass the `-short` tests.
This diff enables `websteps` to use uTLS for TLS parroting. It integrates the `oohttp.StdlibTransport` wrapper which uses the `ooni/oohttp` fork. `oohttp` supports TLS-like connections like `utls.Conn`.
As a prototype, the testhelper and `websteps` code now uses the `utls.HelloChrome_Auto` fingerprint, i.e. the simulated TLS fingerprint of the Google Chrome browser.
It is a further contribution for my GSoC project.
Reference issue: https://github.com/ooni/probe/issues/1733
This is the extension of https://github.com/ooni/probe-cli/pull/431, and my final deliverable for GSoC 2021.
The diff introduces:
1) The new `testhelper` which supports testing multiple IP endpoints per domain and introduces HTTP/3 control measurements. The specification of the `testhelper` can be found at https://github.com/ooni/spec/pull/219. The `testhelper` algorithm consists of three main steps:
* `InitialChecks` verifies that the input URL can be parsed, has an expected scheme, and contains a valid domain name.
* `Explore` enumerates all the URLs that it discovers by redirection from the original URL, or by detecting h3 support at the target host.
* `Generate` performs a step-by-step measurement of each discovered URL.
2) A prototype of the corresponding new experiment `websteps` which uses the control measurement of the `testhelper` to know which URLs to measure, and what to expect. The prototype does not yet have:
* unit and integration tests,
* an analysis tool to compare the control and the probe measurement.
This PR is my final deliverable as it is the outcome of the trials, considerations and efforts of my GSoC weeks at OONI.
It fully integrates HTTP/3 (QUIC) support which has been only used in the `urlgetter` experiment until now.
Related issues: https://github.com/ooni/probe/issues/1729 and https://github.com/ooni/probe/issues/1733.
The utility of SafeErrWrapperBuilder is low. Let us instead change the
code to always create ErrWrapper when we're in this package.
While there, also note some TODO-next items.
Part of https://github.com/ooni/probe/issues/1505.
* refactor(errorsx): start hiding private details and moving around stuff
Part of https://github.com/ooni/probe/issues/1505
* fix: remove now-addressed todo comments
* refactor: cleaner way of passing a UDPConn around
Also part of https://github.com/ooni/probe/issues/1505
* Update internal/engine/netx/quicdialer/connectionstate.go
I needed to add some tests as integration tests due to circular
imports, but this is ~fine because we quite likely want many
integration tests in the errorsx package anyway.
Part of https://github.com/ooni/probe/issues/1505.
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>
This change allows us to have all fasts tests together. They are
mostly unit tests or integration tests that do not require the
network. The advantage of this strategy is the following. We can
now run all these tests with a single click in VSCode. In turn,
doing that tells us which lines of code we are not covering.
The tests requiring the network are in a separate file, so we can
easily see which lines of code are testing without using the network
and which ones instead depend on that. (Currently, 100% of the
inputloader.go file is tested without using the network.)
While there, rename the other file such that is clear that it
contains tests requiring the network. We now have some tests in
inputloader_test.go that are not strictly unit tests.
This refactoring was identified as useful while working
on https://github.com/ooni/probe/issues/1299.
We used to have an external package called libminiooni so that
third parties could use it. We wrote this such that we could
support github.com/bassosimone/aladdin.
That was actually a not-so-good idea because it added to the APIs
we needed to maintain.
Since the merge of engine into cli, such an API is not public
anymore and aladdin has been deprecated and archived.
Therefore, we can now cleanup the situation and merge libminiooni
into miniooni again, thus making the codebase more local.
This cleanup has been identified while working on
https://github.com/ooni/probe/issues/1299.
* ongoing work
* reduce diff with master
* feat(inputloader): use the check-in API
Part of https://github.com/ooni/probe/issues/1299
* fix: better naming for a variable
* chore: add more tests
* fix: add one more TODO
* feat(session): expose CheckIn method
It seems to me the right thing to do is to query the CheckIn API
from the Session rather than querying it from InputLoader.
Then, InputLoader could just take a reference to a Session-like
interface that allows this functionality.
So, this diff exposes the Session.CheckIn method.
Doing that, in turn, required some refactoring to allow for
more and better unit tests.
While doing that, I also noticed that Session required a mutex
to be a well-behaving type, so I did that.
While doing that, I also tried to cover all the lines in session.go
and, as part of that, I have removed unused code.
Reference issue: https://github.com/ooni/probe/issues/1299.
* fix: reinstate comment I shan't have removed
* fix: repair broken test
* fix: a bit more coverage, annotations, etc.
* Update internal/engine/session.go
* Update internal/engine/session_integration_test.go
* Update internal/engine/session_internal_test.go
There was a face-palming error in the implementation causing the proxy
check to be implemented also without a proxy.
This meant that we were ALWAYS skipping http3 and system resolvers.
The bug has been introduced in 3.8.0. So, the currently released
version of the probe, sadly, has this beheavior :-(.
Reference issue https://github.com/ooni/probe/issues/1426.
We use ExperimentOrchestraClient in several places to help us
calling probe-services APIs. We need to call CheckIn because we
want to use CheckIn in InputLoader.
(We also want to remove the URLs API, but that is not something
doable now, since the mobile app is still using this API via
the wrappers at pkg/oonimkall.)
Work part of https://github.com/ooni/probe/issues/1299.
* fix(webconnectivity): expose network events
By not exposing network events in webconnectivity, we are missing
several interesting, explanatory data points.
This diff fixes the issue by:
1. enriching the definition of network events to include extra
data useful for performing (manual) data analysis;
2. adding a tags field to network events such that we can add
tags to specific events and understand where they come from;
3. exposing all the (tagged) network events that happen when running
a webconnectivity experiment.
See https://github.com/ooni/probe-engine/issues/1157.
* progress
* more work towards landing this diff
* Apply suggestions from code review
* Add signal to the im test group
* fix(ipconfig_test.go): disable when running in CI
Reference issue: https://github.com/ooni/probe/issues/1418
* fix(geolocate): remove unused variable
Came across this while looking into this issue with the CI that
is now failing. Guess fixing it here comes across as leaving the
camp slightly less in a bad shape than how I found it.
Co-authored-by: Simone Basso <bassosimone@gmail.com>
* fix(geolocate): no proxy when discovering our IP address
The use case of --proxy is that you cannot contact the OONI
backend otherwise. It is wrong, though, using the proxy when
discovering our IP address. The measurement won't use the
proxy anyway. Therefore, we need to use the IP address that
is performing the measurement. Not the one of the proxy.
What's more, stun is not using a proxy. Therefore, it does
not make much sense that http IP resolvers use a proxy. This
leads to inconsistencies. So, here's anothe reason why this
patch is a good thing (TM).
Finally, because knowing the IP address enables us to sanitize
the data, it's important we discover the correct IP.
Now, up until this point, the `--proxy` option has mostly
been a developers toy. But, users have asked us to have the
possibility of configuring a proxy.
This explains why I have been looking into making `--proxy`
right for a couple of hours now.
See https://github.com/ooni/probe/issues/1382
* fix(session): properly configure the IP lookupper
In reality, we are not going to use the sessionresolver when we're
using a proxy (I just tested). But, it nonetheless feels a lot more
robust to write a correct sessionresolver that handles the proxy
in the most correct way. That is, the sessionresolver will now skip
all the entries that cannot use a socks5 proxy (including among them
also the system resolver). What's more, it will construct a child
resolver that propagates the proxy.
We have confidence that this holds true because we have added a test
ensuring that we are really using the configured proxy.
See https://github.com/ooni/probe/issues/1381
This comes just a few days after 3.6.0. It contains small
improvements required by ooni/probe-desktop.
For this reason, I am going to skeep the normal release
process and I am just bumping the version number.
* fix(webconnectivity): allow measuring https://1.1.1.1
There were two issues preventing us from doing so:
1. in netx, the address resolver was too later in the resolver
chain. Therefore, its result wasn't added to the events.
2. when building the DNSCache (in httpget.go), we didn't consider
the case where the input is an address. We need to treat this
case specially to make sure there is no DNSCache.
See https://github.com/ooni/probe/issues/1376.
* fix: add unit tests for code making the dnscache
* fix(netx): make sure all tests pass
* chore: bump webconnectivity version
* internal/engine/ooapi: auto-generated API client
* feat: introduce the callers abstraction
* feat: implement API caching on disk
* feat: implement cloneWithToken when we require login
* feat: implement login
* fix: do not cache all APIs
* feat: start making space for more tests
* feat: implement caching policy
* feat: write tests for caching layer
* feat: add integration tests and fix some minor issues
* feat: write much more unit tests
* feat: add some more easy unit tests
* feat: add tests that use a local server
While there, make sure many fields we care about are OK.
* doc: write basic documentation
* fix: tweak sentence
* doc: improve ooapi documentation
* doc(ooapi): other documentation improvements
* fix(ooapi): remove caching for most APIs
We discussed this topic yesterday with @FedericoCeratto. The only
place where we want LRU caching is MeasurementMeta.
* feat(ooapi): improve handling of errors during login
This was also discussed yesterday with @FedericoCeratto
* fix(swaggerdiff_test.go): temporarily disable
Before I work on this, I need to tend onto other tasks.
* fix(ootest): add one more test case
We're going towards 100% coverage of this package, as it ought to be.
* feat(ooapi): test cases for when the probe clock is off
* fix(ooapi): change test to have 100% unittest coverage
* feat: sync server and client APIs definition
Companion PR: https://github.com/ooni/api/pull/218
* fix(ooapi): start testing again against API
* fix(ooapi): only generate each file once
* chore: set version to 3.7.0-alpha
While there, make sure we don't always skip a currently failing
riseupvpn test, and slightly clarify the readme.
* fix(kvstore): less scoped error message
* chore: update dependencies
* chore: update user agent for measurements
* chore: we're now at v3.6.0
* chore: update assets
* chore: update bundled CA
* fix: address some goreportcard.com warnings
* fix(debian/changelog): zap release that breaks out build scripts
We're forcing the content of changelog with `dch`, so it's fine to
not have any specific new release in there.
* fix: make sure tests are passing locally
Notably, I removed a chunk of code where we were checking for network
activity. Now we don't fetch the databases and it's not important. Before,
it was important because the databases are ~large.
* fix: temporarily comment out riseupvn integration tests
See https://github.com/ooni/probe/issues/1354 for work aimed at
reducing the rate of false positives (thanks @cyBerta!)
* feat(sessionresolver): try many and use what works
* fix(sessionresolver): make sure we can use quic
* fix: the config struct is unnecessary
* fix: make kvstore optional
* feat: write simple integration test
* feat: start adding tests
* feat: continue writing tests
* fix(sessionresolver): add more unit tests
* fix(sessionresolver): finish adding tests
* refactor(sessionresolver): changes after code review
* feat: use go1.16 embedding for resources
We want to embed everything that can be easily embedded. We should, at a
minimum, replace the downloading of resources and bindata.
Ref: https://github.com/ooni/probe/issues/1367.
* fix: get rid of bindata and use go embed instead
* fix: start unbreaking some automatic tests
* fix: fetch resources as part of the mobile build
* fix: convert more stuff to go1.16
I still expect many breakages, but we'll fix them.
* fix: make the windows CI green
* fix: get resources before running QA
* fix: go1.16 uses modules by default
* hopefully fix all other outstanding issues
* fix(QA/telegram.py): add another DC IP address
* Apply suggestions from code review
* MVP of a signal messenger test
* Add minimal signal test unit tests
* Add Signal test to the im nettest group
* Add test for https://sfu.voip.signal.org/
* Fix bug in client-side determination of blocking status
* Add uptime.signal.org to the test targets
* Add more tests
* Check for invalid CA being passed
* Check that the update function works as expected
* Update internal/engine/experiment/signal/signal_test.go
Co-authored-by: Simone Basso <bassosimone@gmail.com>
* fix: back out URL we shouldn't have changed
When merging probe-engine into probe-cli, we changed too many URLs
and some of them should not have been changed.
I noticed this during the review of Signal and I choose to add
this commit to revert such changes.
While there, make sure the URL of the experiment is OK.
* fix(signal): reach 100% of coverage
Just so that we can focus on areas of the codebase where we need
more coverage, let us avoid missing an easy line to test.
Co-authored-by: Simone Basso <bassosimone@gmail.com>
