RiseupVPN tests improvements

[cyBerta]

Checklist

• [x] I have read the contribution guidelines
• [x] reference issue for this pull request: https://github.com/ooni/probe/issues/1928 and https://github.com/ooni/probe/issues/1842
• [x] if you changed anything related to 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/273
• [x] if you changed code inside an experiment, make sure you bump its version number

Description

This MR aims to improve the data quality of RiseupVPN tests by reduce the amount of false positives due to server side issues. Major changes are:

• API failures are reported, but not necessarily a reason to stop the test early
• invalid CA's are indicated but not considered a failure and the tests will continue without TLS verification
• a blocked API will be indicated as well, but tried to be circumvented with a Tor+Snowflake tunnel.
• the API is only considered blocked if the main configuration end point is unreachable, RiseupVPN has multiple ways to get around other other API blockages or server side misbehavior (targets https://github.com/ooni/probe/issues/1842#issuecomment-952783818)
• overloaded gateways are not tested anymore
• only a subset of gateways are tested

[cyBerta]

Hm... the coverage CI action wasn't failing before, when I played with the code on my personal github repo and filed a MR to myself.

From the above failing report - RiseupVPN's coverage:
github.com/ooni/probe-cli/v3/internal/experiment/riseupvpn 0.684s coverage: 99.4% of statements

[cyBerta]

Hi @bassosimone @hellais, is there anything I could help with to support the review process?

[cyBerta]

@bassosimone I've rebased the MR to that latest master branch. I'm sure you know that: to test the MR, you'll need to build mini-ooni. proble-cli tests will fail without an redirection in go.mod or until the changes landed on master. We would love to see this MR getting in soon, it will help us a lot with improving measurements for LEAP VPN / RiseupVPN.

[bassosimone]

@cyBerta I will take care of doing a review of this MR! Thank you for updating riseupvpn!

[bassosimone]

Thank you for your patience @cyBerta! I am currently finishing the review, running tests, and generally think about this experiment and how we can move it forward! I estimate I'll publish the review next week. 🙏

[cyBerta]

Wow, Thanks for this extensive review! I appreciate a lot the time you took for it. I can definitely change the given MR according to your short term proposals, they sound reasonable to me. Also awesome that you apparently already started to work on the midterm proposals as I understand https://github.com/ooni/probe/issues/2502. I haven't spent time yet to dig into the DSL approach, but will do so soon, so that I can hopefully help to maintain the shiny new solution.

[bassosimone]

Thank you, @cyBerta !

[cyBerta]

ok, I think I've addressed your proposals.

1. We drop all the top-level keys from the experiment and we only keep the results generated by urlgetter.

I've left 2 TestKeys that helps within the test to use tls-no-verify in case fetching the CA cert fails or the CA is expired. Next to the slow response time of Riseup's API server it was a common case for false positives. Also we're using these keys in a fetch-and-parsing pipeline for prometheus, so keeping these keys would help us to change less code at other places as well.

At the same time, we simplify the code that generates the summary so that it always says that there was no anomaly.

I've removed the summary keys and the tests always return false for IsAnomaly

This change removes the possibility that individual users would be triggered by flaky behavior of api.black.riseup.net and shifts the focus on the experiment purely on collecting data that should only be analyzed at scale (like we do for dnscheck).

yey, should be the case now

2. We change the logic of the experiment such that, if any of the APIs required to bootstrap fail, we stop and submit a smaller JSON to the backend. This change removes the extra complexity that was already there and the additional one you introduced in this commit. Arguably, this would also make the experiment less complete than the one you implemented in this pull request, but I also have a plan to address that. However, this plan is not for the short term, but luckily it intersects with my other work commitments. I expand on this plan in detail below.

I've changed the test so that it fails and returns early.

3. We modify the code to avoid submitting the JSON returned by the geo service, given that it includes geolocation data that it would be better not to submit. Granted, there is a mechanism in place with ooniprobe that strips the user’s IP address from the measurement. But I can see a bunch of cases where different geolocation mechanisms (the one used by OONI and the one used by riseup) may not agree because the IP address is different for some reasons and I would not like for us to increase the likelihood of a leak.

while I was at it I've removed all response bodies, since they aren't interesting for us and they unnecessarily leak data

[bassosimone]

I mistakenly pushed some of my commits in this branch, but I reverted it now.

[bassosimone]

@cyBerta I started merging code from this PR into the master branch. The related PRs are https://github.com/ooni/probe-cli/pull/1360, https://github.com/ooni/probe-cli/pull/1363, and https://github.com/ooni/probe-cli/pull/1364.

https://github.com/ooni/probe-cli/pull/1363#issue-1938109948 in particular explains my rationale for pulling changes and what I chose not to include for the 3.19 release. It also explains how I think we can use some form of richer input, such as the DSL described above, or perhaps something simpler (and easier to implement), to decouple testing services useful to bootstrap riseupvpn and getting the endpoints we should be testing.

I think this PR should probably remain open because it contains the work about finding out which subset of the gateways to test, which I think in the future should be an algorithm that runs in the backend to find out which gateways probes should test.

If you have no objections, I'd like to go ahead and merge your changes with master, so it's more clear what has not been added into master and the algorithms about parsing the geo service results stand out more clearly.

Thank you!

[cyBerta]

@cyBerta I started merging code from this PR into the master branch. The related PRs are #1360, #1363, and #1364.

#1363 (comment) in particular explains my rationale for pulling changes and what I chose not to include for the 3.19 release. It also explains how I think we can use some form of richer input, such as the DSL described above, or perhaps something simpler (and easier to implement), to decouple testing services useful to bootstrap riseupvpn and getting the endpoints we should be testing.

I think this PR should probably remain open because it contains the work about finding out which subset of the gateways to test, which I think in the future should be an algorithm that runs in the backend to find out which gateways probes should test.

If you have no objections, I'd like to go ahead and merge your changes with master, so it's more clear what has not been added into master and the algorithms about parsing the geo service results stand out more clearly.

Thank you!

hi @bassosimone Thank you so much for your thoughts, effort and work you've put into this review and the subsequent PRs! I'm curious to see how this test suite evolves. please let me know how I can support the development in any way.