Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add echcheck to the experimental suite #1217

Merged
merged 12 commits into from
Oct 11, 2023

Conversation

kelmenhorst
Copy link
Contributor

@kelmenhorst kelmenhorst commented Aug 30, 2023

Checklist

Description

This diff does two things:

  1. Add echcheck to the experimental nettest suite. We do not provide input which causes the experiment to use the default URL https://crypto.cloudflare.com/cdn-cgi/trace (URL proposed by @eighthave). With this single input configuration we can collect the first experimental ECH measurements.
  2. Add netem tests to echcheck, while keeping a "real-internet" test to connect to an ECH-enabled server.

@eighthave
Copy link

I don't know this codebase, or Go for that matter, so I can't really follow this. But the idea sounds good. I would recommend including the data that https://crypto.cloudflare.com/cdn-cgi/trace returns, especially the results of the sni= field. For example:

fl=125f76
h=crypto.cloudflare.com
ip=23.153.248.38
ts=1693413463.195
visit_scheme=https
uag=Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0
colo=MCI
sliver=none
http=http/2
loc=T1
tls=TLSv1.3
sni=plaintext
warp=off
gateway=off
rbi=off
kex=X25519

You can see my Firefox does not have ECH enabled, since sni=plaintext

@kelmenhorst
Copy link
Contributor Author

@eighthave Yes I was thinking the same. However, at the moment, echcheck only handshakes. That said, I don't see a reason why we should not do HTTP GET here.

@bassosimone bassosimone changed the title Enable ECH as part of the experimental nettest suite chore: enable ECH as part of the experimental nettest suite Oct 11, 2023
Conflicts:
	internal/experiment/echcheck/measure_test.go
@bassosimone bassosimone changed the title chore: enable ECH as part of the experimental nettest suite chore: add echcheck to the experimental suite Oct 11, 2023
@bassosimone bassosimone changed the title chore: add echcheck to the experimental suite feat: add echcheck to the experimental suite Oct 11, 2023
@bassosimone
Copy link
Contributor

bassosimone commented Oct 11, 2023

@kelmenhorst the reason why we should not fetch the webpage here was that the result included the IP address of the probe and we would like to avoid that. We used to have a mitigation where we scrubbed the IP address known to us, i.e., the one we discovered, from the measurement result. Since v3.19.0-alpha, there's now a much more robust mitigation where we scrub any IP address or endpoint, which means now it is much safer to fetch this page.

To clarify: we discover one of the probe's IPv4 and IPv6 addresses and tend to prefer IPv4 during DNS lookups, so the previous scrubbing policy has been fine in 99% of the cases. The new policy ensures we cover all possible cases.

Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐳

@bassosimone bassosimone merged commit 39019d0 into ooni:master Oct 11, 2023
7 checks passed
bassosimone added a commit that referenced this pull request Oct 11, 2023
This diff backports #1217 to the release/3.19 branch.

- [x] I have read the [contribution
guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md)
- [x] reference issue for this pull request:
ooni/probe#1453
ooni/probe#2547
- [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/kelmenhorst/spec/tree/echcheck-spec
- [x] if you changed code inside an experiment, make sure you bump its
version number

This diff does two things:

1. Add `echcheck` to the experimental nettest suite. We do not provide
input which causes the experiment to use the default URL
`https://crypto.cloudflare.com/cdn-cgi/trace` (URL
[proposed](ooni/probe#1453 (comment))
by @eighthave). With this single input configuration we can collect the
first experimental ECH measurements.

2. Add netem tests to `echcheck`, while keeping a "real-internet" test
to connect to an ECH-enabled server.

---------

Co-authored-by: Simone Basso <[email protected]>
@bassosimone bassosimone mentioned this pull request Oct 11, 2023
26 tasks
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this pull request Feb 13, 2024
## 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:
ooni/probe#1453
ooni/probe#2547
- [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/kelmenhorst/spec/tree/echcheck-spec
- [x] if you changed code inside an experiment, make sure you bump its
version number

## Description

This diff does two things:

1. Add `echcheck` to the experimental nettest suite. We do not provide
input which causes the experiment to use the default URL
`https://crypto.cloudflare.com/cdn-cgi/trace` (URL
[proposed](ooni/probe#1453 (comment))
by @eighthave). With this single input configuration we can collect the
first experimental ECH measurements.

2. Add netem tests to `echcheck`, while keeping a "real-internet" test
to connect to an ECH-enabled server.

---------

Co-authored-by: Simone Basso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants