This change ensures that, in turn, we're able to "remote" all the traffic generated by the `geolocate` package, rather than missing some bits of it that were still using the standard library and caused _some_ geolocations to geolocate as the local host rather than as the remote host.
Extracted from https://github.com/ooni/probe-cli/pull/969, where we tested this functionality.
Closes https://github.com/ooni/probe/issues/1383 (which was long overdue).
Part of https://github.com/ooni/probe/issues/2340, because it allows us to make progress with that.
A bunch of packages (including oohelperd) just need the ability to
use MaxMind-like databases. They don't need the additional functionality
implemented by the geolocate package. Such a package, in fact, is
mostly (if not only) needed by the engine package.
Therefore, move code to query MaxMind-like databases to a separate
package, and avoid depending on geolocate in all the packages for
which it's sufficient to use geoipx.
Part of https://github.com/ooni/probe/issues/2240
This diff refactors netx and netxlite to ensure we're not using
netxlite legacy names inside of netx.
To this end, we're cheating a bit. We're exposing a new factory to
get an unwrapped stdlib resolver rather than defining a legacy name
to export the private name of the same factory.
This is actually a fine place to stop, for now, the next and
netxlite refactoring at https://github.com/ooni/probe/issues/2121.
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
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 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?).
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.
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.
## 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
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
## 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
* 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
* 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
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
* 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
* 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
* 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
* refactor: move more commands to internal/cmd
Part of https://github.com/ooni/probe/issues/1335.
We would like all commands to be at the same level of engine
rather than inside engine (now that we can do it).
* fix: update .gitignore
* refactor: also move jafar outside engine
* We should be good now?
* refactor: start building an Android package
Part of https://github.com/ooni/probe/issues/1335.
This seems also a good moment to move some packages out of the
engine, e.g., oonimkall. This package, for example, is a consumer
of the engine, so it makes sense it's not _inside_ it.
* fix: committed some stuff I didn't need to commit
* fix: oonimkall needs to be public to build
The side effect is that we will probably need to bump the major
version number every time we change one of these APIs.
(We can also of course choose to violate the basic guidelines of Go
software, but I believe this is bad form.)
I have no problem in bumping the major quite frequently and in
any case this monorepo solution is convinving me more than continuing
to keep a split between engine and cli. The need to embed assets to
make the probe more reliable trumps the negative effects of having to
~frequently bump major because we expose a public API.
* fix: let's not forget about libooniffi
Honestly, I don't know what to do with this library. I added it
to provide a drop in replacement for MK but I have no idea whether
it's used and useful. I would not feel comfortable exposing it,
unlike oonimkall, since we're not using it.
It may be that the right thing to do here is just to delete the
package and reduce the amount of code we're maintaining?
* woops, we're still missing the publish android script
* fix(publish-android.bash): add proper API key
* ouch fix another place where the name changed
This is how I did it:
1. `git clone https://github.com/ooni/probe-engine internal/engine`
2. ```
(cd internal/engine && git describe --tags)
v0.23.0
```
3. `nvim go.mod` (merging `go.mod` with `internal/engine/go.mod`
4. `rm -rf internal/.git internal/engine/go.{mod,sum}`
5. `git add internal/engine`
6. `find . -type f -name \*.go -exec sed -i 's@/ooni/probe-engine@/ooni/probe-cli/v3/internal/engine@g' {} \;`
7. `go build ./...` (passes)
8. `go test -race ./...` (temporary failure on RiseupVPN)
9. `go mod tidy`
10. this commit message
Once this piece of work is done, we can build a new version of `ooniprobe` that
is using `internal/engine` directly. We need to do more work to ensure all the
other functionality in `probe-engine` (e.g. making mobile packages) are still WAI.
Part of https://github.com/ooni/probe/issues/1335
