This diff modifies netx to use netxlite to build the TLSDialer.
Building the TLSDialer entails building a TLSHandshaker.
While there, hide netxlite names we don't want to be public
and change netx tests to test for functionality.
To this end, refactor filtering to provide an easier to
use TLS server. We don't need the complexity of proxying
rather we need to provoke specific errors.
Part of https://github.com/ooni/probe/issues/2121
By just storing the raw certificate we simplify the internal data
structure we use. In turn, this enables us to write better unit tests
using github.com/google/go-cmp where we can construct the expected
result and compare with that. (Yeah, in principle we could also
construct the full certificate but I'm not sure it's worth the effort
since we basically only care about the raw certificate.)
The general idea here is to make tracex more tested. Once it's more
tested, I will create separate structs for each event, which is
something that measurex also does. Once that is done, we can start
ensuring that the code in measurex and the code in tracex do the
same thing in terms of storing observations. When also this is done,
we can then rewrite measurex to use tracex directly.
The overall goal is https://github.com/ooni/probe/issues/2035.
There are two reasons why this is beneficial:
1. github.com/google/go-cmp is more annoying to use for comparing
data structures when there are interfaces to compare. Sure, there's
a recipe for teaching it to compare errors, but how about making
the errors trivially comparable instead?
2. if we want to send errors over the network, JSON serialization
works but we cannot unmarshal the resulting string back to an error,
so how about making this representation trivial to serialize (we
are not going this now, but we need this property for websteps and
it may be sensible to try to avoid to have duplicate code because
of that -- measurex currently duplicates many tracex functionality
and this is quite unfortunate because it slows development down)
Additionally, if an error is a string:
3. we can very easily use a switch for comparing its possible
values with "" representing the absence of errors, while it is
more complex to do the same when using a nullable string or even
an error (i.e., an interface)
4. if a type is not nullable, it's easier to write safe code for
it and we may want to refactor experiments to use the internal
representation of measurements for more robust processing code
For all these reasons, let's internally use strings in tracex.
The overall aim here is to reduce the duplicated code between pre
and post-measurex measurements (see https://github.com/ooni/probe/issues/2035).
This diff forward ports b606494db8a9293384efaf5c33a88601f6e1e2a6
to the main development branch.
Dnscheck is emitting progress and the experiment controller is
also emitting progress. This messes up the progress bar.
See https://github.com/ooni/probe/issues/2058#issuecomment-1141638067
* refactor: move tracex outside of engine/netx
Consistently with https://github.com/ooni/probe/issues/2121 and
https://github.com/ooni/probe/issues/2115, we can now move tracex
outside of engine/netx. The main reason why this makes sense now
is that the package is now changed significantly from the one
that we imported from ooni/probe-engine.
We have improved its implementation, which had not been touched
significantly for quite some time, and converted it to unit
testing. I will document tomorrow some extra work I'd like to
do with this package but likely could not do $soon.
* go fmt
* regen tutorials
The exercise already allowed me to notice issues such as fields not
being properly initialized by savers.
This is one of the last steps before moving tracex away from the
internal/netx package and into the internal package.
See https://github.com/ooni/probe/issues/2121
Tracex contained some fragile code that assembled HTTP measurements
from scattered events, which worked because we were sure we were
performing a single measurement at any given time.
This diff restructures the code to emit a transaction-start and a
transaction-done events only. We have basically removed all the other
events (which we were not using). We kept the transaction-start
though, because it may be useful to see it when reading events. In
any case, what matters here is that we're now using the transaction-done
event aline to generate the archival HTTP measurement.
Hence, the original issue has been addressed. We will possibly
do more refactoring in the future, but for now this seems sufficient.
Part of https://github.com/ooni/probe/issues/2121
The main issue I see inside tracex at the moment is that we
construct the HTTP measurement from separate events.
This is fragile because we cannot be sure that these events
belong to the same round trip. (Currently, they _are_ part
of the same round trip, but this is a fragile assumption and
it would be much more robust to dispose of it.)
To prepare for emitting a single event, it's imperative to
have two distinct fields for HTTP request and response headers,
which is the main contribution in this commit.
Then, we have a bunch of smaller changes including:
1. correctly naming 'response' the DNS response (instead of 'reply')
2. ensure we always use pointer receivers
Reference issue: https://github.com/ooni/probe/issues/2121
Rather than matching a string, match a type.
This is more robust considering future refactorings.
We're confident the names did not change in _this_ refactoring
because we're still testing the same strings in the tests.
Part of https://github.com/ooni/probe/issues/2121
Acknowledge that transports MAY be used in isolation (i.e., outside
of a Resolver) and add support for wrapping.
Ensure that every factory that creates an unwrapped type is named
accordingly to hopefully ensure there are no surprises.
Implement DNSTransport wrapping and use a technique similar to the
one used by Dialer to customize the DNSTransport while constructing
more complex data types (e.g., a specific resolver).
Ensure that the stdlib resolver's own "getaddrinfo" transport (1)
is wrapped and (2) could be extended during construction.
This work is part of my ongoing effort to bring to this repository
websteps-illustrated changes relative to netxlite.
Ref issue: https://github.com/ooni/probe/issues/2096
This diff modifies the system resolver to use a getaddrinf transport.
Obviously the transport is a fake, but its existence will allow us
to observe DNS events more naturally.
A lookup using the system resolver would be a ANY lookup that will
contain all the resolved IP addresses into the same response.
This change was also part of websteps-illustrated, albeit the way in
which I did it there was less clean than what we have here.
Ref issue: https://github.com/ooni/probe/issues/2096
Rather than passing functions to construct complex objects such
as Dialer and QUICDialer, pass interface implementations.
Ensure that a nil implementation does not cause harm.
Make Saver implement the correct interface either directly or
indirectly. We need to implement the correct interface indirectly
for TCP conns (or connected UDP sockets) because we have two
distinct use cases inside netx: observing just the connect event
and observing just the I/O events.
With this change, the construction of composed Dialers and
QUICDialers is greatly simplified and more obvious.
Part of https://github.com/ooni/probe/issues/2121
The code that is now into the tracex package was written a long
time ago, so let's start to make it more in line with the coding
style of packages that were written more recently.
I didn't apply all the changes I'd like to apply in a single diff
and for now I am committing just this diff.
Broadly, what we need to do is:
1. improve documentation
2. ~always use pointer receivers (object receives have the issue
that they are not mutable by accident meaning that you can mutate
them but their state do not change after the call returns, which
is potentially a source of bugs in case you later refactor to use
a pointer receiver, so always use pointer receivers)
3. ~always avoid embedding (let's say we want to avoid embedding
for types we define and it's instead fine to embed types that are
defined in the stdlib: if later we add a new method, we will not
see a broken build and we'll probably forget to add the new method
to all wrappers -- conversely, if we're wrapping rather than
embedding, we'll see a broken build and act accordingly)
4. prefer unit tests and group tests by type being tested rather
than using a flat structure for tests
There's a coverage slippage that I'll compensate in a follow-up diff where I'll focus on unit testing.
Reference issue: https://github.com/ooni/probe/issues/2121
This diff creates a new package under netx called tracex that
contains everything we need to perform measurements using events
tracing and postprocessing (which is the technique with which
we implement most network experiments).
The general idea here is to (1) create a unique package out of
all of these packages; (2) clean up the code a bit (improve tests,
docs, apply more recent code patterns); (3) move the resulting
code as a toplevel package inside of internal.
Once this is done, netx can be further refactored to avoid
subpackages and we can search for more code to salvage/refactor.
See https://github.com/ooni/probe/issues/2121
This diff modifies the construction of a dialer to allow one
to insert custom dialer wrappers into the dialers chain.
The point of the chain in which we allow custom wrappers is the
optimal one for connect, read, and write measurements.
This new design is better than the previous netx design since
we don't need to construct the whole chain manually now.
The work in this diff is part of the effort to make engine/netx
just a tiny wrapper around netxlite.
See https://github.com/ooni/probe/issues/2121.
This diff required us to move some code around, but no major
change actually happened, except better tests.
While there, I also slightly refactored ndt7's implementation and
removed the ProxyURL setting, which was actually unused.
See https://github.com/ooni/probe/issues/2121
This diff replaces engine/netx code with netxlite code in
the engine/session.go file. To this end, we needed to move
some code from engine/netx to netxlite. While there, we
did review and improve the unit tests.
A notable change in this diff is (or seems to be) that in
engine/session.go we're not filtering for bogons anymore so
that, in principle, we could believe a resolver returning
to us bogon IP addresses for OONI services. However, I did
not bother with changing bogons filtering because the
sessionresolver package is already filtering for bogons,
so it is actually okay to avoid doing that again the
session.go code. See:
https://github.com/ooni/probe-cli/blob/v3.15.0-alpha.1/internal/engine/internal/sessionresolver/resolvermaker.go#L88
There are two reference issues for this cleanup:
1. https://github.com/ooni/probe/issues/2115
2. https://github.com/ooni/probe/issues/2121
In https://github.com/ooni/probe/issues/2029#issuecomment-1140805266, we
explained why calling it "netgo" would be incorrect.
In other words, we can get the platform's `getaddrinfo` as long as
we're not cross compiling. We do cross compile `ooniprobe`, actually
it's not even possible to cross compile it.
For increased accuracy, we should stop cross compiling `miniooni`
as well, so it would also directly use `getaddrinfo`.
This diff fixes at the same time ooni/probe-cli and ooni/spec
and we'll open two pull requests in parallel.
After https://github.com/ooni/probe-cli/pull/764, the build for
CGO_ENABLED=0 has been broken for miniooni:
https://github.com/ooni/probe-cli/runs/6636995859?check_suite_focus=true
Likewise, it's not possible to run tests with CGO_ENABLED=0.
To make tests work with `CGO_ENABLED=0`, I needed to sacrifice some
unit tests run for the CGO case. It is not fully clear to me what was happening
here, but basically `getaddrinfo_cgo_test.go` was compiled with CGO
being disabled, even though the ``//go:build cgo` flag was specified.
Additionally, @hellais previously raised a valid point in the review
of https://github.com/ooni/probe-cli/pull/698:
> Another issue we should consider is that, if I understand how
> this works correctly, depending on whether or not we have built
> with CGO_ENABLED=0 on or not, we are going to be measuring
> things in a different way (using our cgo inspired getaddrinfo
> implementation or using netgo). This might present issues when
> analyzing or interpreting the data.
>
> Do we perhaps want to add some field to the output data format that
> gives us an indication of which DNS resolution code was used to
> generate the the metric?
This comment is relevant to the current commit because
https://github.com/ooni/probe-cli/pull/698 is the previous
iteration of https://github.com/ooni/probe-cli/pull/764.
So, while fixing the build and test issues, let us also distinguish
between the CGO_ENABLED=1 and CGO_ENABLED=0 cases.
Before this commit, OONI used "system" to indicate the case where
we were using net.DefaultResolver. This behavior dates back to the
Measurement Kit days. While it is true that ooni/probe-engine and
ooni/probe-cli could have been using netgo in the past when we
said "system" as the resolver, it also seems reasonable to continue
to use "system" top indicate getaddrinfo.
So, the choice here is basically to use "netgo" from now on to
indicate the cases in which we were built with CGO_ENABLED=0.
This change will need to be documented into ooni/spec along with
the introduction of the `android_dns_cache_no_data` error.
## 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/2029
- [x] if you changed anything related how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: https://github.com/ooni/spec/pull/242
To this end, we need to refactor the implementation to give the
DNSOverUDPChannel owenership over the net.Conn.
Once this happens, DNSOverUDPChannel.Close closes the conn.
When the conn is closed, the background goroutine will terminate
immediately because any blocking I/O operation will be immediately
unblocked and return net.ErrClosed.
See https://github.com/ooni/probe/issues/2099#issuecomment-1139066946
This diff introduces support for observing additional DNS-over-UDP
responses in some censored environments (e.g. China).
After some uncertainty around whether to use connected or unconnected
UDP sockets, I eventually settled for connected.
Here's a recap:
| | connected | unconnected |
| ----------------------- | --------- | ----------- |
| see ICMP errors | ✔️ | ❌ |
| responses from any server | ❌ | ✔️ |
Because most if not all DNS resolvers expect answers from exactly
the same servers to which they sent the query, I would say that
it's more important to have some limited ability of observing the
effect of ICMP errors (e.g., host_unreachable when we set a low
TTL and send out a query to a server).
Therefore, my choice was to modify the existing DNS-over-UDP transport.
Here's an overview of the changes:
1. introduce a new API for performing an async round trip that returns
a channel wrapper where all responses are posted. The channel will not ever
be closed, so the reader needs to use select for safely reading. If the
reader users the wrapper's Next or TryNextResponses methods, these details
do not matter because they already implement a safe reading pattern.
2. the async round trip API performs the round trip in the background
and stops processing when it sees the first error.
3. the background running code will use an overall deadline derived
from the DNSTransport.IOTimeout field to know when to stop.
4. the background running code will additionally stop running if
noone is reading the channel and there are no empty slots in the
channel's buffer.
5. the RoundTrip method has been rewritten in terms of the async API.
The design I'm using here implements the proposal for async round
trips defined at https://github.com/ooni/probe/issues/2099. I have
chosen not to make all transports async because the DNS transport
seems the only transport that needs to also work in async mode.
While there, I noticed that we were not propagating CloseIdleConnection
to the underlying dialer, which was potentially wrong, so I did it.
This diff refactors the DNSTransport model to receive in input a DNSQuery and return in output a DNSResponse.
The design of DNSQuery and DNSResponse takes into account the use case of a transport using getaddrinfo, meaning that we don't need to serialize and deserialize messages when using getaddrinfo.
The current codebase does not use a getaddrinfo transport, but I wrote one such a transport in the Websteps Winter 2021 prototype (https://github.com/bassosimone/websteps-illustrated/).
The design conversation that lead to producing this diff is https://github.com/ooni/probe/issues/2099
These two small packages could easily be merged into the model
package, since they're clearly model-like packages.
Part of https://github.com/ooni/probe/issues/2115
The objective is to make PR checks run much faster.
See https://github.com/ooni/probe/issues/2113 for context.
Regarding netxlite's tests:
Checking for every commit on master or on a release branch is
good enough and makes pull requests faster than one minute since
netxlite for windows is now 1m slower than coverage.
We're losing some coverage but coverage from integration tests
is not so good anyway, so I'm not super sad about this loss.
Rather than building for Android using ooni/go, we're now using
ooni/oocryto as the TLS dependency. Such a repository only forks
crypto/tls and some minor crypto packages and includes the
same set of patches that we have been using in ooni/go.
This new strategy should be better than the previous one in
terms of building for Android, because we can use the vanilla
go1.18.2 build. It also seems that it is easier to track and
merge from upstream with ooni/oocrypto than it is with ooni/go.
Should this assessment be wrong, we can revert back to the
previous scenario where we used ooni/go.
See https://github.com/ooni/probe/issues/2106 for extra context.
* Passed the TestHelpers field to RunAsyc and MeasureAsync. This reflects the test_helpers in the measurement.
* Spec already contains the correct output.
See https://github.com/ooni/probe/issues/2073
Co-authored-by: decfox <decfox>
Previously, the DNS decoder did not check whether it was parsing
a DNS query or a DNS response, which was wrong.
As a side note, it seems I am using "reply" in the codebase instead
of "response". The latter seems correct DNS terminology.
This diff has been extracted from 9249d14f80
See https://github.com/ooni/probe/issues/2096.
This diff has been extracted and adapted from 8848c8c516
The reason to prefer composition over embedding is that we want the
build to break if we add new methods to interfaces we define. If the build
does not break, we may forget about wrapping methods we should
actually be wrapping. I noticed this issue inside netxlite when I was working
on websteps-illustrated and I added support for NS and PTR queries.
See https://github.com/ooni/probe/issues/2096
While there, perform comprehensive netxlite code review
and apply minor changes and improve the docs.
This diff has been extracted from c2f7ccab0e
See https://github.com/ooni/probe/issues/2096
While there, export DecodeReply to decode a raw reply without
interpreting the Rcode or parsing the results, which seems a
nice extra feature to have to more flexibly parse DNS replies
in other parts of the codebase.
This error occurred for example when querying kazemjalali.com
in websteps measurements run from Iran.
This error is relatively uncommon, but it still makes sense to
create a specific mapping rule for it.
Originally: 4269e82fbd
See https://github.com/ooni/probe/issues/2096
I've seen some measurements returning some IP addresses for HTTPSSvc
queries but not returning any ALPN value.
For example:
```
% d4
decoding DNS round trip 0:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 57768
;; flags: rd; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0
;; QUESTION SECTION:
;psiphon.ca. IN HTTPS
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 57768
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0
;; QUESTION SECTION:
;psiphon.ca. IN HTTPS
;; ANSWER SECTION:
psiphon.ca. 121 IN A 31.13.85.53
```
Now, the response is clearly bogus. At the time of this writing that
IP address belongs to Facebook. This measurement has been collected in
China, so it's expected for the GFW to behave like this.
Yet, I don't feel like it's accurate to report this measurement as a
"no answer" response. Rather, this response is a valid one containing
a clearly invalid IP address and should be flagged as such.
Originally: 57a023bcf4
See https://github.com/ooni/probe/issues/2096
This diff fixes the short-circuit-IP-addr resolver to
correctly handle IP addrs during LookupHTTPS.
The original diff was: 2b51d144bf
See https://github.com/ooni/probe/issues/2096
While there, add unit tests for IPv6.
This diff re-implements the vanilla_tor experiment. This experiment was
part of the ooni/probe-legacy implementation.
The reference issue is https://github.com/ooni/probe/issues/803. We didn't
consider the possible improvements mentioned by the
https://github.com/ooni/probe/issues/803#issuecomment-598715694 comment,
which means we'll need to create a follow-up issue for them. We will
then decide whether, when, and how to implement those follow-up measurements
either into `vanilla_tor` or into the existing `tor` experiment.
This novel `vanilla_tor` implementation emits test_keys that are mostly
compatible with the original implementation, however:
1. the `timeout` is a `float64` rather than integer (but the default
timeout is an integer, so there are no JSON-visible changes);
2. the `tor_log` string is gone and replaced by the `tor_logs` list
of strings, which contains the same information;
3. the definition of `error` has been augmented to include the
case in which there is an unknown error;
4. the implementation of vanilla_tor mirrors closely the one of torsf
and we have taken steps to make the two implementations as comparable
as possible in terms of the generated JSON measurement.
The main reason why we replaced `tor_log` with `tor_logs` are:
1. that `torsf` already used that;
2. that reading the JSON is easier with this implementation compared to
an implementation where all logs are into the same string.
If one is processing the new data format using Python, then it will
not be difficult convert `tor_log` to `tor_logs`. In any case, because
we extract the most interesting fields (e.g., the percentage of the
bootstrap where tor fails), it seems that logs are probably more useful
as something you want to read in edge cases (I guess).
Also, because we want `torsf` and `vanilla_tor` to have similar JSONs,
we renamed `torsf`'s `default_timeout` to `timeout`. This change has little
to none real-world impact, because no stable version of OONI Probe has
ever shipped a `torsf` producing the `default_timeout` field.
Regarding the structure of this diff, we have:
1. factored code to parse tor logs into a separate package;
2. implemented `vanilla_tor` as a stripped down `torsf` and added further
changes to ensure compatibility with the previous `vanilla_tor`'s data format;
3. improved `torsf` to merge back the changes in `vanilla_tor`, so the two
data formats of the two experiments are as similar as possible.
We believe producing as similar as possible data formats helps anyone who's
reading measurements generated by both experiments.
We have retained/introduced `vanilla_tor`'s `error` field, which is not very
useful when one has a more precise failure but is still what `vanilla_tor`
used to emit, so it makes sense to also have this field.
In addition to changing the implementation, we also updated the specs.
As part of our future work, we may want to consider factoring the common code
of these two experiments into the same underlying support library.
* quic-go upgrade: replaced Session/EarlySession with Connection/EarlyConnection
* quic-go upgrade: added context to RoundTripper.Dial
* quic-go upgrade: made corresponding changes to tutorial
* quic-go upgrade: changed sess variable instances to qconn
* quic-go upgrade: made corresponding changes to tutorial
* cleanup: remove unnecessary comments
Those comments made sense in terms of illustrating the changes
but they're going to be less useful once we merge.
* fix(go.mod): apparently we needed `go1.18.1 mod tidy`
VSCode just warned me about this. It seems fine to apply this
change as part of the pull request at hand.
* cleanup(netxlite): http3dialer can be removed
We used to use http3dialer to glue a QUIC dialer, which had a
context as its first argument, to the Dial function used by the
HTTP3 transport, which did not have a context as its first
argument.
Now that HTTP3 transport has a Dial function taking a context as
its first argument, we don't need http3dialer
anymore, since we can use the QUIC dialer directly.
Cc: @DecFox
* Revert "cleanup(netxlite): http3dialer can be removed"
This reverts commit c62244c620cee5fadcc2ca89d8228c8db0b96add
to investigate the build failure mentioned at
https://github.com/ooni/probe-cli/pull/715#issuecomment-1119450484
* chore(netx): show that test was already broken
We didn't see the breakage before because we were not using
the created transport, but the issue of using a nil dialer was
already present before, we just didn't see it.
Now we understand why removing the http3transport in
c62244c620cee5fadcc2ca89d8228c8db0b96add did cause the
breakage mentioned at
https://github.com/ooni/probe-cli/pull/715#issuecomment-1119450484
* fix(netx): convert broken integration test to working unit test
There's no point in using the network here. Add a fake dialer that
breaks and ensure we're getting the expected error.
We've now improved upon the original test because the original test was
not doing anything while now we're testing whether we get back a QUIC
dialer that _can be used_.
After this commit, I can then readd the cleanup commit
c62244c620cee5fadcc2ca89d8228c8db0b96add and it won't be
broken anymore (at least, this is what I expected to happen).
* Revert "Revert "cleanup(netxlite): http3dialer can be removed""
This reverts commit 0e254bfc6ba3bfd65365ce3d8de2c8ec51b925ff
because now we should have fixed the broken test.
Co-authored-by: decfox <decfox>
Co-authored-by: Simone Basso <bassosimone@gmail.com>
* tls_handshakes: add IP addresses
* tls_handshakes: extract ip from tcp-connect
* tls_handshake: switched to trace event
* saver.go: get remoteAddr before handshake
Not sure whether this is strictly necessary, but I'd rather take the
remoteAddr before calling Handshake, just in case a future version
of the handshake closes the `conn`. In such a case, `conn.RemoteAddr`
would return `nil` and we would crash here.
This occurred to me while reading once again the diff before merging.
Co-authored-by: decfox <decfox>
Co-authored-by: Simone Basso <bassosimone@gmail.com>
This diff changes the software name used by unattended runs for which
we did not override the default software name (`ooniprobe-cli`).
It will become `ooniprobe-cli-unattended`. This software name is in line
with the one we use for Android, iOS, and desktop unattended runs.
While working in this diff, I introduced string constants for the run
types and a string constant for the default software name.
See https://github.com/ooni/probe/issues/2081.
Here's the squash of the following patches that enable support
for go1.18 and update our dependencies.
This diff WILL need to be backported to the release/3.14 branch.
* chore: use go1.17.8
See https://github.com/ooni/probe/issues/2067
* chore: upgrade to probe-assets@v0.8.0
See https://github.com/ooni/probe/issues/2067.
* chore: update dependencies and enable go1.18
As mentioned in 7a0d17ea91,
the tree won't build with `go1.18` unless we say it does.
So, not only here we need to update dependencies but also we
need to explicitly say `go1.18` in the `go.mod`.
This work is part of https://github.com/ooni/probe/issues/2067.
* chore(coverage.yml): run with go1.18
This change will give us a bare minimum confidence that we're
going to build our tree using version 1.18 of golang.
See https://github.com/ooni/probe/issues/2067.
* chore: update user agent used for measuring
See https://github.com/ooni/probe/issues/2067
* chore: run `go generate ./...`
See https://github.com/ooni/probe/issues/2067
* fix(dialer_test.go): make test work with go1.17 and go1.18
1. the original test wanted the dial to fail, so ensure we're not
passing any domain name to exercise dialing not resolving;
2. match the end of the error rather than the whole error string.
Tested locally with both go1.17 and go1.18.
See https://github.com/ooni/probe-cli/pull/708#issuecomment-1096447186
This change should prevent old clients (e.g., Android 6) from
failing to perform a ndt7 experiment because their internal CA
bundle is now too old.
Reference issue: https://github.com/ooni/probe/issues/2031
While there, run `go mod tidy` to fix a minor inconsistence in
the current `go.mod` file.
This diff WILL require a backport to release/3.14.
This experiment pings a QUIC-able host. It can be used to measure QUIC availability independently from TLS.
This is the reference issue: https://github.com/ooni/probe/issues/1994
### A QUIC PING is:
- a QUIC Initial packet with a size of 1200 bytes (minimum datagram size defined in the [RFC 9000](https://www.rfc-editor.org/rfc/rfc9000.html#initial-size)),
- with a random payload (i.e. no TLS ClientHello),
- with the version string 0xbabababa which forces Version Negotiation at the server.
QUIC-able hosts respond to the QUIC PING with a Version Negotiation packet.
The input is a domain name or an IP address. The default port used by quicping is 443, as this is the port used by HTTP/3. The port can be modified with the `-O Port=` option.
The default number of repetitions is 10, it can be changed with `-O Repetitions=`.
### Usage:
```
./miniooni -i google.com quicping
./miniooni -i 142.250.181.206 quicping
./miniooni -i 142.250.181.206 -OPort=443 quicping
./miniooni -i 142.250.181.206 -ORepetitions=2 quicping
```
The oonireport client (re-)uploads a measurement report file. This can be helpful when the measurement was not uploaded at runtime.
Usage: `./oonireport upload <file>`, where `<file>` is a json(l) file containing one OONI measurement per line.
This pull request refers to https://github.com/ooni/probe/issues/2003 and https://github.com/ooni/probe/issues/950.
Co-authored-by: Simone Basso <bassosimone@gmail.com>
This commit forward ports 8f2d7945f806579af4d0495f4b8f5a6a01eefb0c, whose
commit message is as follows:
- - -
The discrepancy I was seeing between my local tests and tests run
in the CI is that my systemd is configured to use DoT.
Hence, it was bypassing iptables rules because the query was sent
over an encrypted tunnel. Using a pure Go resolver fixes since
that always uses UDP, so the filter works.
Also, reason that we want as minimal as possible tests, so refactor
a test so that we use just a resolver rather than an HTTP client, and,
while there, also enforce this resolver to be a pure Go resolver.
Reference issue: https://github.com/ooni/probe/issues/2016
This diff WILL need to be forward ported to master.
This diff forward ports b6db4f64dc83a2a27ee3ce6bba5ac93db922832d, whose
original log message is the following:
- - -
We're now using ooni/oohttp as our HTTP library in most cases.
A limitation of this library is that net/http/httptrace does not
work very well and reliably because (1) we need to use oohttp's
version of that code and (2) we cannot observe net events.
I noticed this fact because an integration test for collecting
HTTP performance metrics was broken.
The best solution here is to remove this functionality, since
it was basically unused in the repository. Only some integration
tests inside urlgetter bothered with these metrics.
A more clinical fix would have been to use ooni/oohttp/httptrace
instead of net/http/httptrace in the stdlib, but it does not
seem to be a good idea, given that those metrics were not used.
With this diff applied, we'll further reduce the number of locally
failing integration tests to just jafar-specific tests.
This diff WILL need to be forwardported to `master`.
We've just branched off the release/3.14 branch for finalizing
the release of 3.14.0, hence let's declare that from now on we're
3.15.0-alpha to avoid any confusion.
This issue aims at making life slighly better for users impacted by
sanctions whose iplookup may be quite slow in case there are timeouts
as documented in https://github.com/ooni/probe/issues/1988.
Cloudflare hosted services provide a certain service of `/cdn-cgi/trace` with their base url (for example, `www.cloudflare.com` or `www.nginx.com`), which can be used to obtain `ip` in the probe's `geolocate` feature.
The same feature was added in this pr, hence, increasing the number of `baseURL`s in `geolocate`.
Co-authored-by: Simone Basso <bassosimone@gmail.com>
This diff contains significant improvements over the previous
implementation of the torsf experiment.
We add support for configuring different rendezvous methods after
the convo at https://github.com/ooni/probe/issues/2004. In doing
that, I've tried to use a terminology that is consistent with the
names being actually used by tor developers.
In terms of what to do next, this diff basically instruments
torsf to always rendezvous using domain fronting. Yet, it's also
possible to change the rendezvous method from the command line,
when using miniooni, which allows to experiment a bit more. In the
same vein, by default we use a persistent tor datadir, but it's
also possible to use a temporary datadir using the cmdline.
Here's how a generic invocation of `torsf` looks like:
```bash
./miniooni -O DisablePersistentDatadir=true \
-O RendezvousMethod=amp \
-O DisableProgress=true \
torsf
```
(The default is `DisablePersistentDatadir=false` and
`RendezvousMethod=domain_fronting`.)
With this implementation, we can start measuring whether snowflake
and tor together can boostrap, which seems the most important thing
to focus on at the beginning. Understanding why the bootstrap most
often does not converge with a temporary datadir on Android devices
remains instead an open problem for now. (I'll also update the
relevant issues or create new issues after commit this.)
We also address some methodology improvements that were proposed
in https://github.com/ooni/probe/issues/1686. Namely:
1. we record the tor version;
2. we include the bootstrap percentage by reading the logs;
3. we set the anomaly key correctly;
4. we measure the bytes send and received (by `tor` not by `snowflake`, since
doing it for snowflake seems more complex at this stage).
What remains to be done is the possibility of including Snowflake
events into the measurement, which is not possible until the new
improvements at common/event in snowflake.git are included into a
tagged version of snowflake itself. (I'll make sure to mention
this aspect to @cohosh in https://github.com/ooni/probe/issues/2004.)
I have tested this integration test locally and it's now WAI.
It may be that it will fail again when run on GitHub Actions, which will
indicate we cannot fully trust Actions for running _some_ tests.
Closes https://github.com/ooni/probe/issues/1913.
This diff includes some final changes to be ready for blessing
a cli release. These changes are:
1. run `go generate ./...` to update the bundled CA
2. update the header we use for measuring
3. ensure `mk` uses the latest version of several tools
Reference issue: https://github.com/ooni/probe/issues/1845
This commit message is the same across probe-cli, probe-desktop,
and probe-android. With the changes contained in the enclosed
diff, I'm starting to add support for torsf for android and for
desktop.
When smoke testing that torsf was WAI, I also noticed that its
progress messages in output are too frequent. We may want to do
better in a future version when we'll be able to read `tor`'s
output. In the meanwhile, make the progress messages less
frequent and indicated the maximum runtime inside of the messages
themselves. This improved message, albeit not so nice from the
UX PoV, should at least provide a clue that we're not stuck.
Reference issue: https://github.com/ooni/probe/issues/1917
Reference issue: https://github.com/ooni/probe/issues/1917.
I needed to change the summary key type returned by `torsf` to be a value. It seems the DB layer assumes that. If we pass it a pointer, it panics because it's experiment a value rather than a pointer 🤷.
I have experimented with a new approach for embedding psiphon in
7fc0bcd97c.
It seems the build is still building and the experiment is still
running. With the new approach, we're now vendoring less dependencies,
which hopefully puts us in the right track to, one day, import
Psiphon as a normal Go dependency.
I'll make sure to report to the Psiphon team what is currently
preventing us from importing their ClientLibrary directly.
This work is part of https://github.com/ooni/probe/issues/1894.
As part of running the update, I run `go get -u -v ./...`, which
led to go-cmp also being updated in the process.
This diff contains the following changes and enhancements:
1. upgrade snowflake to v2
2. observe that we were not changing defaults from outside of snowflake.go, so remove code allowing to do that;
3. bump the timeout to 600 seconds (it seems 300 was not always enough based on my testing);
4. add useful knob to disable `torsf` progress (it's really annoying on console, we should do something about this);
5. ptx.go: avoid printing an error when the connection has just been closed;
6. snowflake: test AMP cache, see that it's not working currently, so leave it disabled.
Related issues: https://github.com/ooni/probe/issues/1845, https://github.com/ooni/probe/issues/1894, and https://github.com/ooni/probe/issues/1917.
This diff introduces a new package called `./internal/archival`. This package collects data from `./internal/model` network interfaces (e.g., `Dialer`, `QUICDialer`, `HTTPTransport`), saves such data into an internal tabular data format suitable for on-line processing and analysis, and allows exporting data into the OONI data format.
The code for collecting and the internal tabular data formats are adapted from `measurex`. The code for formatting and exporting OONI data-format-compliant structures is adapted from `netx/archival`.
My original objective was to _also_ (1) fully replace `netx/archival` with this package and (2) adapt `measurex` to use this package rather than its own code. Both operations seem easily feasible because: (a) this code is `measurex` code without extensions that are `measurex` related, which will need to be added back as part of the process; (b) the API provided by this code allows for trivially converting from using `netx/archival` to using this code.
Yet, both changes should not be taken lightly. After implementing them, there's need to spend some time doing QA and ensuring all nettests work as intended. However, I am planning a release in the next two weeks, and this QA task is likely going to defer the release. For this reason, I have chosen to commit the work done so far into the tree and defer the second part of this refactoring for a later moment in time. (This explains why the title mentions "1/N").
On a more high-level perspective, it would also be beneficial, I guess, to explain _why_ I am doing these changes. There are two intertwined reasons. The first reason is that `netx/archival` has shortcomings deriving from its original https://github.com/ooni/netx legacy. The most relevant shortcoming is that it saves all kind of data into the same tabular structure named `Event`. This design choice is unfortunate because it does not allow one to apply data-type specific logic when processing the results. In turn, this choice results in complex processing code. Therefore, I believe that replacing the code with event-specific data structures is clearly an improvement in terms of code maintainability and would quite likely lead us to more confidently change and evolve the codebase.
The second reason why I would like to move forward these changes is to unify the codepaths used for measuring. At this point in time, we basically have two codepaths: `./internal/engine/netx` and `./internal/measurex`. They both have pros and cons and I don't think we want to rewrite whole experiments using `netx`. Rather, what we probably want is to gradually merge these two codepaths such that `netx` is a set of abstractions on top of `measurex` (which is more low-level and has a more-easily-testable design). Because saving events and generating an archival data format out of them consists of at least 50% of the complexity of both `netx` and `measurex`, it seems reasonable to unify this archival-related part of the two codebases as the first step.
At the highest level of abstraction, these changes are part of the train of changes which will eventually lead us to bless `websteps` as a first class citizen in OONI land. Because `websteps` requires different underlying primitives, I chose to develop these primitives from scratch rather than wrestling with `netx`, which used another model. The model used by `websteps` is that we perform each operation in isolation and immediately we save the results, while `netx` creates whole data structures and collects all the events happening via tracing. We believe the model used by `websteps` to be better because it does not require your code to figure out everything that happened after the measurement, which is a source of subtle bugs in the current implementation. So, when I started implementing websteps I extracted the bits of `netx` that could also be beneficial to `websteps` into a separate library, thus `netxlite` was born.
The reference issue describing merging the archival of `netx` and `measurex` is https://github.com/ooni/probe/issues/1957. As of this writing the issue still references the original plan, which I could not complete by the end of this Sprint, so I am going to adapt the text of the issue to only refer to what was done in here next. Of course, I also need follow-up issues.
* chore(netxlite): add currently failing test case
This diff introduces a test cases that will fail because of the reason
explained in https://github.com/ooni/probe/issues/1965.
* chore(netxlite/iox_test.go): add failing unit tests
These tests directly show how the Go implementation of ReadAll
and Copy has the issue of checking for io.EOF equality.
* fix(netxlite): make {ReadAll,Copy}Context robust to wrapped io.EOF
The fix is simple: we just need to check for `errors.Is(err, io.EOF)`
after either io.ReadAll or io.Copy has returned. When this condition is
true, we need to convert the error back to `nil` as it ought to be.
While there, observe that the unit tests I committed in the previous
commit are wrongly asserting that the error must be wrapped. This
assertion is not correct, because in both cases we have just ensured
that the returned error is `nil` (i.e., success).
See https://github.com/ooni/probe/issues/1965.
* cleanup: remove previous workaround for wrapped io.EOF
These workarounds were partial, meaning that they would cover some
cases in which the issue occurred but not all of them.
Handling the problem in `netxlite.{ReadAll,Copy}Context` is the
right thing to do _as long as_ we always use these functions instead
of `io.{ReadAll,Copy}`.
This is why it's now important to ensure we clearly mention that
inside of the `CONTRIBUTING.md` guide and to also ensure that we're
not using these functions in the code base.
* fix(urlgetter): repair tests who assumed to see EOF error
Now that we have established that we should normalize EOF when
reading bodies like the stdlib does and now that it's clear why
our behavior diverged from the stdlib, we also need to repair
all the tests that assumed this incorrect behavior.
* fix(all): don't use io{,util}.{Copy,ReadAll}
* feat: add checks to ensure we don't use io.{Copy,ReadAll}
* doc(netxlite): document we know how to deal w/ wrapped io.EOF
* fix(nocopyreadall.bash): add exception for i/n/iox.go
The DNSClient type existed because the Resolver type did not
include CloseIdleConnections in its signature.
Now that Resolver includes CloseIdleConnections, the DNSClient
type has become unnecessary and can be safely removed.
See https://github.com/ooni/probe/issues/1956.
We recently started moving core data structures inside of the
internal/model package as detailed in https://github.com/ooni/probe/issues/1885.
The chief reason to do that is to have a set of fundamental
shared data types to help us rationalize the codebase.
This specific diff moves internal/netx/archival's core data types
inside the internal/model package. While there, it also refactors the
existing tests to improve their quality. Additionally, we also added
an extra test to ensure `ArchivalHTTPBody` is an alias for
`ArchivalMaybeBinaryData`, which is required to ensure the
custom JSON serialization process works for it.
We're doing that because both internal/netx/archival and
internal/measurex define their own archival data structures.
We developed measurex using its own structures because it
allowed to iterate more quickly. Now that we have sketched
out measurex, the time has come to consolidate.
My overall aim is to spend a few more hours this week on
engineering measurex. This work is preliminary work before
we finish up both measurex and websteps.
We described this cleanup in https://github.com/ooni/probe/issues/1957.
This diff addresses two items of https://github.com/ooni/probe/issues/1956:
> - [ ] we can remove legacy names from `./internal/engine/netx/resolver/legacy.go`
>
> - [ ] we can remove `DialTLSContext` from `./internal/engine/netx/resolver/tls_test.go`
More cleanups may follow.
This is another cleanup point mentioned by https://github.com/ooni/probe/issues/1956.
While there, fix a bunch of comments in jafar that were incorrectly
referring to the netx package name.
This diff addresses another point of https://github.com/ooni/probe/issues/1956:
> - [ ] observe that we're still using a bunch of private interfaces for common interfaces such as the `Dialer`, so we can get rid of these private interfaces and always use the ones in `model`, which allows us to remove a bunch of legacy wrappers
Additional cleanups may still be possible. The more I cleanup, the more I see
there's extra legacy code we can dispose of (which seems good?).
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
