feat: add the openvpn experiment #1585

ainghazal · 2024-05-02T14:54:18Z

Description

First iteration of a new openvpn experiment. This takes a set of endpoints and uses minivpn to attempt a handshake with each of the configured endpoints.

A new API endpoint has been added to the backend that is able to distribute valid configuration parameters for an OpenVPN connection, including credentials.

Checklist

I have read the contribution guidelines
reference issue for this pull request: vpn: ship MVP for the openvpn experiment probe#2688
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: feat: add openvpn experiment spec spec#293
if you changed code inside an experiment, make sure you bump its version number

ainghazal · 2024-06-03T17:25:01Z

tests green again after latest additions.
ooni/probe#2741 blocks testing using miniooni, but I can provide a local implementation of probe-services to test this.

bassosimone

My main, still open, design question is about whether we would be able to easily disable the experiment via the OONI backend API if it misbehaves. This is my main reason for saying "request changes". My understanding is that, if the OONI backend does not return any target to measure, then the experiment will use the defaults configured for "riseupvpn". I do not think this is desirable at this stage, because we still think at this experiment as unstable and we would like to have a chance to configure it to not run remotely. How could we achieve this? Would removing the hardcoded fallback be a possibility?

Apart from that, I have done a careful review that includes:

several minimal cosmetic requests including documenting public fields
suggestions and questions about simplifying the codebase a bit
small design questions

bassosimone · 2024-06-04T08:27:31Z

internal/engine/inputloader.go

+	for _, provider := range openvpn.APIEnabledProviders {
+		reply, err := il.vpnConfig(ctx, provider)
+		if err != nil {
+			break


If you want to stop at the first error, you should return an error here. Looking at the context of the code, it seems to me the semantics to want is to ignore (and maybe warn?) on error. To this end, you should continue here.

tx for pointing this out! this for loop is not really used atm, so I'll go with returning the error

bassosimone · 2024-06-04T08:28:02Z

internal/engine/inputloader.go

+		// here we're just collecting all the inputs. we also cache the configs so that
+		// each experiment run can access the credentials for a given provider.


I don't see where you're caching the configs in the following two lines

you're right - cacheing happens in the session method. updated the comment.

internal/experiment/openvpn/endpoint.go

bassosimone · 2024-06-04T09:34:55Z

internal/experiment/openvpn/endpoint.go

+	},
+}
+
+// Shuffle returns a shuffled copy of the endpointList.


Is it really a shuffled copy? It seems to me this is shuffling e itself.

To make a shuffled copy you need to do something like this:

copy := append([]endpointList{}, e...)

and then you need to do a rand.Shuffle on copy (obviously).

Also, I'd rename this method Shuffled (like Python's sort and sorted).

noted. I changed the docstring to reflect that we mutate the list in place.
in any case, I'll probably stop using the default endpoint list, so probably this code needs to be deleted soon.

bassosimone · 2024-06-04T10:06:27Z

internal/experiment/openvpn/openvpn_test.go

+}
+
+func TestAddConnectionTestKeys(t *testing.T) {
+	t.Run("append connection result to empty keys", func(t *testing.T) {


Please, add a test for the UDP case as well

internal/experiment/openvpn/openvpn_test.go

ainghazal · 2024-06-04T16:45:15Z

For disabling the experiment: would this work? 8c5fa11

would this be handled correctly by the mobile/desktop apps just by returning an error?

right now if the HTTP client finds a 404 miniooni panics btw, I'm not sure if I need to catch this error explicitely.

ainghazal · 2024-06-04T16:46:28Z

addressed the comments in the review; I think the only case I'd still defend is adding a boolean for success (in the top-level OpenVPN handshake archival result, I have removed the nested result struct) since I think simplifies analysis in some cases (ooni data can always override it)

do note that after agreeing on the failure/success format, I'll need to modify the spec accordingly!

ainghazal · 2024-06-04T18:24:38Z

tests seem to be failing for an unrelated reason

ainghazal · 2024-06-05T15:52:15Z

Added a couple of commits, one for accepting options in the oonirun descriptor (not used at the moment), and the other using measurexlite.NewFailure. This can be tested with the following oonirun file:

{
  "name": "openvpn-riseup",
  "description": "measure vpn connection to riseup gateway",
  "author": "Ain Ghazal <ain@openobservatory.org>",
  "nettests": [
    {
      "test_name": "openvpn",
      "inputs": [
        "openvpn://riseupvpn.corp/?address=51.15.187.53:53&transport=udp"
      ],
      "options": {
        "Cipher": "AES-256-GCM",
        "Auth": "SHA512",
        "Compress": "",
        "Obfuscation": "none",
        "SafeCA": "-----BEGIN CERTIFICATE-----\nMIIBYjCCAQigAwIBAgIBATAKBggqhkjOPQQDAjAXMRUwEwYDVQQDEwxMRUFQIFJv\nb3QgQ0EwHhcNMjExMTAyMTkwNTM3WhcNMjYxMTAyMTkxMDM3WjAXMRUwEwYDVQQD\nEwxMRUFQIFJvb3QgQ0EwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQxOXBGu+gf\npjHzVteGTWL6XnFxtEnKMFpKaJkA/VOHmESzoLsZRQxt88GssxaqC01J17idQiqv\nzgNpedmtvFtyo0UwQzAOBgNVHQ8BAf8EBAMCAqQwEgYDVR0TAQH/BAgwBgEB/wIB\nATAdBgNVHQ4EFgQUZdoUlJrCIUNFrpffAq+LQjnwEz4wCgYIKoZIzj0EAwIDSAAw\nRQIgfr3w4tnRG+NdI3LsGPlsRktGK20xHTzsB3orB0yC6cICIQCB+/9y8nmSStfN\nVUMUyk2hNd7/kC8nL222TTD7VZUtsg==\n-----END CERTIFICATE-----",
        "SafeCert": "-----BEGIN CERTIFICATE-----\nMIICdzCCAh2gAwIBAgIQaiLN8Wt8B7n/fkNAb0dPhjAKBggqhkjOPQQDAjAzMTEw\nLwYDVQQDDChMRUFQIFJvb3QgQ0EgKGNsaWVudCBjZXJ0aWZpY2F0ZXMgb25seSEp\nMB4XDTI0MDUyMzExNDYzOFoXDTI0MDYyNzExNDYzOFowFDESMBAGA1UEAxMJVU5M\nSU1JVEVEMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA6/FnGI7yIw0P\nb1p+IRiTrLi+lc2bacboPE//bsTFBh+PuTudyZeGY9mxZDd1wBauwBux8nnSnaKK\nJGzd+5xQrG9xlb19aMSBWpS8+rdCz03zEG31SQgwSAAUjmYsDEhstwY329K7noCZ\n125Dp5HIPZYRS5Xbc5NirazrRongsYdq53Mlmp2S/KPFBo6LkkNppBcSzIo6wsUo\n96PViTy15yA4PWhrI5dP798Wg7od4R5Q/cM+zyaxoycwKUck4Ixhqw7nod8OBXZu\nwITKGGqFmMtAQAa8FZG1y+e85O7GH55K/W/3sO5T4fcV+A9V5AyynRQfZbxvIT0I\nbMX1/y7+WwIDAQABo2cwZTAOBgNVHQ8BAf8EBAMCB4AwEwYDVR0lBAwwCgYIKwYB\nBQUHAwIwHQYDVR0OBBYEFJ81OtiqvnHEnXngCTa2wpyPeOhKMB8GA1UdIwQYMBaA\nFH1KYtj/K0nEebaisdyOPmMHXKj+MAoGCCqGSM49BAMCA0gAMEUCIBmbOvPti2bM\nEkjHRoCRE4h09da1YMDdYhYgndmnQaeMAiEAs0RqX86JTcB2M4FkW+gw1ni3Okuc\ntBKwmlxGufWxzpE=\n-----END CERTIFICATE-----",
        "SafeKey": "-----BEGIN RSA PRIVATE KEY-----\nMIIEogIBAAKCAQEA6/FnGI7yIw0Pb1p+IRiTrLi+lc2bacboPE//bsTFBh+PuTud\nyZeGY9mxZDd1wBauwBux8nnSnaKKJGzd+5xQrG9xlb19aMSBWpS8+rdCz03zEG31\nSQgwSAAUjmYsDEhstwY329K7noCZ125Dp5HIPZYRS5Xbc5NirazrRongsYdq53Ml\nmp2S/KPFBo6LkkNppBcSzIo6wsUo96PViTy15yA4PWhrI5dP798Wg7od4R5Q/cM+\nzyaxoycwKUck4Ixhqw7nod8OBXZuwITKGGqFmMtAQAa8FZG1y+e85O7GH55K/W/3\nsO5T4fcV+A9V5AyynRQfZbxvIT0IbMX1/y7+WwIDAQABAoIBAHLmFUmtWxdcpdaZ\nX/DoEgo70XwMK5Hgbnnoj0C3DCeGOSyAbr+cTbLUcYGXTH1lzmX5Vrf5QWrIm7NP\nXO7J2bOPdeXw6GCbyU5+PmVt11gy4ppuodOV7EUz3M7XzL2Si3a5zXv8bKesgr6Z\nkNLKuJPdP8DqUns/G//txImOXWC13QFk0KGeEzWU5/5tXR8SP/wft68sYQiNmfPw\nxGIuqIs/ILsEYM/vXirBx3Q4KyfrJZ71X3ueRKxhxnMAN6wztYqwa7A9NjSMu566\npjG2c0tBXEk+IrX4ehXk/+tM9oCHM+gV19uOky56mTjNQEetFibMbd8u/5zF/pga\niu+KEBECgYEA/qklejvogI4jFA0pwK//3gYMostn+hq4pM3yBHGgxNuJu2PQr7oP\nZ8KUaHIoZOb/FD2E/JuYkpA6raIWVd/7R+oVwq8OVPpKxtAtjD9u4+gHvkpDF1dv\nNkwHSeG9+9EOcNeqCHzGLK6VJys0FVEBOJSWgGy5xsjvFsYpbrLaJdMCgYEA7S8O\na9RoA0AEr3B/zcrGRKZceyF/z7maOYEUuZL8Cf4GxSciQN5DddcYXl5yzjFUiubV\ncM4kzqvQfBrVYFhNAHOPVQ4Unp+UzEWKe3juPnYVJOh0GLreW0nSC2QMftSZrspC\nlgDr2d3q06tAXDNKo0mko6b4MQF6GzdpNTcpyFkCgYAxutpEuno20IrtGXzz0erH\ncqr5B3uwjZNNK6J9V6srhiupWl6gUlc7zfWpR9G3kpxxWWok4kWzKVMsISD3eBvb\n+UxyjjjgQ1hi5rheUOzYuLD6agob/skK82Hg/aJaEIMfah4cNjGE/DrIQVmUaBMy\n92FEhvboaMi3y86/fVG4XQKBgBBRhX97vLBEjk33woNJKTz96Sz7kAydq3O7Ys6l\nwzt4w8R6vcuSvzdzVhTgEKwJDtUDrrm1JSkm/xAa1IVtbdbTHJBwiJClUBqBylZW\naqXXf/rrF1nAOZ40RQRNnOJ5BB3Xgp9JbvCtaQOpK6NsT/1OCsrLqRXOETWgKVfk\n9LX5AoGAAuuOw6PNpKvef595GzTjsXuGDH7PtVkr4CP5j7qDCKh1t7rjk3RiHzTH\nsyXNZQIsRagE3lo32t5ORhW0X+2oGjAS/YkHKDo/ueGqEzBbWtVcDZKNoxfnjCZ6\ngBNUSqhYykBGDI0ST56CnG0ZJSxztncR39W2TrxpYju3qy1g3Ps=\n-----END RSA PRIVATE KEY-----\n"
      }
    }
  ]
}

ainghazal · 2024-06-05T17:57:40Z

updated the spec to reflect the latest change in the archival format: ooni/spec@518c1d5

internal/engine/session.go

bassosimone

I am going to merge such that we can continue development in tree. Notably, this experiment is a very good candidate for reasoning about richer input, hence merging it is going to provide collateral help in completing richer input.

💥 🐳

## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/spec/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: ooni/probe#2688 - [x] related ooni/probe-cli pull request: ooni/probe-cli#1585 - [x] If I changed a spec, I also bumped its version number and/or date ## Description Add initial spec for the `openvpn` experiment. This proposal supersedes a previous version (#255). ICMP Pings and URLGrabber over the tunnel are not included, this experiment just does OpenVPN handshakes at the moment. --------- Co-authored-by: Simone Basso <bassosimone@gmail.com>

ainghazal requested review from bassosimone and hellais as code owners May 2, 2024 14:54

ainghazal mentioned this pull request May 2, 2024

Openvpn 2024 #1553

Closed

4 tasks

ainghazal marked this pull request as draft May 2, 2024 15:20

ainghazal self-assigned this May 10, 2024

ainghazal marked this pull request as ready for review May 10, 2024 05:33

ainghazal added 24 commits May 30, 2024 13:31

stub for openvpn experiment

5c0a415

add hardcoded endpoint

06624b0

update archival format

aaf6995

wip

d4b65c3

hardcode credentials as step zero

c4bb706

flatten test keys

87bbe21

wip

8c2e23e

wip

54c9510

include openvpn options in handshake result

1bc0764

add fields to archival struct for handshake result

567ed14

wip: input

5974d03

api call to fetch credentials

0852d1a

use the newer endpoint, provider in url

8bb671b

wip: fetch config, process input as part of inputloader

1ea507f

parse input in new format

46b40f1

cache config from api

0c6a062

cache per-provider config

199f105

do not parse base64 in creds from api

6ada20f

tests

accaf97

update docs

bb25311

move archival structs to internal/model

dfd8c83

move list of enabled providers to expriment/openvpn

d6d7ed8

do not use credentials

e291c0e

remove test file

8934898

force single provider for now

2a6fc25

bassosimone suggested changes Jun 4, 2024

View reviewed changes

bassosimone changed the title ~~Feat/openvpn 2024~~ feat: introduce the openvpn experiment Jun 4, 2024

ainghazal added 4 commits June 4, 2024 17:25

add changes after code review

ec8396a

move the lock after instantiating the client

b909756

unflatten the failure

b8fbd82

abort experiment if no urls returned

8c5fa11

ainghazal and others added 2 commits June 4, 2024 20:26

Merge remote-tracking branch 'ooni/master' into feat/openvpn-2024-3

330f1aa

Merge branch 'master' into feat/openvpn-2024-3

1ce6835

bassosimone requested a review from DecFox as a code owner June 5, 2024 14:44

ainghazal added 2 commits June 5, 2024 17:41

add stub to receive options from oonirun descriptors

6377ba8

use measurexlite.NewFailure

9b27cc1

ainghazal added 2 commits June 5, 2024 18:05

update tests

196d584

update test after change in default endpoints if api fails

1a60013

bassosimone added 2 commits June 5, 2024 21:19

Merge branch 'master' into feat/openvpn-2024-3

0fae492

fix(registry): update openvpn registration

9fac70c

bassosimone reviewed Jun 6, 2024

View reviewed changes

internal/engine/session.go Outdated Show resolved Hide resolved

Update internal/engine/session.go

5afb8bd

bassosimone approved these changes Jun 6, 2024

View reviewed changes

bassosimone mentioned this pull request Jun 6, 2024

feat: add openvpn experiment spec ooni/spec#293

Merged

4 tasks

bassosimone changed the title ~~feat: introduce the openvpn experiment~~ feat: add the openvpn experiment Jun 6, 2024

bassosimone merged commit 1e4f104 into ooni:master Jun 6, 2024
18 checks passed

BrewTestBot mentioned this pull request Aug 8, 2024

ooniprobe 3.23.0 Homebrew/homebrew-core#180481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add the openvpn experiment #1585

feat: add the openvpn experiment #1585

ainghazal commented May 2, 2024 •

edited

Loading

ainghazal commented Jun 3, 2024 •

edited

Loading

bassosimone left a comment •

edited

Loading

bassosimone Jun 4, 2024

ainghazal Jun 4, 2024

bassosimone Jun 4, 2024

ainghazal Jun 4, 2024

bassosimone Jun 4, 2024

ainghazal Jun 4, 2024

bassosimone Jun 4, 2024

ainghazal Jun 4, 2024

ainghazal commented Jun 4, 2024 •

edited

Loading

ainghazal commented Jun 4, 2024 •

edited

Loading

ainghazal commented Jun 4, 2024 •

edited

Loading

ainghazal commented Jun 5, 2024 •

edited

Loading

ainghazal commented Jun 5, 2024

bassosimone left a comment

		// here we're just collecting all the inputs. we also cache the configs so that
		// each experiment run can access the credentials for a given provider.

feat: add the openvpn experiment #1585

feat: add the openvpn experiment #1585

Conversation

ainghazal commented May 2, 2024 • edited Loading

Description

Checklist

ainghazal commented Jun 3, 2024 • edited Loading

bassosimone left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ainghazal commented Jun 4, 2024 • edited Loading

ainghazal commented Jun 4, 2024 • edited Loading

ainghazal commented Jun 4, 2024 • edited Loading

ainghazal commented Jun 5, 2024 • edited Loading

ainghazal commented Jun 5, 2024

bassosimone left a comment

Choose a reason for hiding this comment

ainghazal commented May 2, 2024 •

edited

Loading

ainghazal commented Jun 3, 2024 •

edited

Loading

bassosimone left a comment •

edited

Loading

ainghazal commented Jun 4, 2024 •

edited

Loading

ainghazal commented Jun 4, 2024 •

edited

Loading

ainghazal commented Jun 4, 2024 •

edited

Loading

ainghazal commented Jun 5, 2024 •

edited

Loading