BREAKING CHANGE: fix: pin certificates to hosts #40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before writing this diff, netem automatically generated the correct cert on the fly based on the given SNI or local addr. While this behavior has been okay so far, it turns out there is a set of tests we cannot write because of it. Namely, we cannot check whether we can connect to a given host using another SNI, because netem would generate a certificate for the provided SNI, which is not how the internet works. If I'm connecting to www.example.com with www.google.com as the SNI, in the internet the server would return a valid certificate for www.example.com, rather than for www.google.com. This diff rectifies netem's behavior by forcing the user to pin the certificate to a set of names and addresses when creating the server that needs such a certificate. Accordingly, we can stop using a forked google/martian/v3/mitm implementation and we can just roll out our own, which is still based on martian (feat not, not reinventing the wheel here) but allows us to create a certificate with specific addresses and domain names pinned to it. While there, notice that there was code we were not using, such as stdlib.go, and that we also don't need in ooni/probe-cli, so we can definitely kill this piece of code. Part of ooni/probe#2531, because the tests I was trying to write belong to such an issue.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this pull request
Sep 20, 2023
As explained in ooni/netem#40, netem used to generate on the fly certificates for any host. I then discovered that this behavior is not desirable because the internet doesn't work like that. Specifically, TLS dialing to, say, www.example.com with www.google.com as the SNI must return a TLS error (`ssl_invalid_hostname` in OONI). This wrong behavior of netem has become a bottleneck for writing new tests for the beacons functionality, so it must change. I have already changed netem and this diff updates the probe-cli tree to use a netem version that behaves more correctly. While there, I noticed that sniblocking tests were wrong because of the previous netem behavior and asserted that the control should return a nil failure, while it should have been `ssl_invalid_hostname`. Part of ooni/probe#2531
bassosimone
added a commit
to ooni/probe-cli
that referenced
this pull request
Sep 20, 2023
As explained in ooni/netem#40, netem used to generate on the fly certificates for any host. I then discovered that this behavior is not desirable because the internet doesn't work like that. Specifically, TLS dialing to, say, www.example.com with www.google.com as the SNI must return a TLS error (`ssl_invalid_hostname` in OONI). This wrong behavior of netem has become a bottleneck for writing new tests for the beacons functionality, so it must change. I have already changed netem and this diff updates the probe-cli tree to use a netem version that behaves more correctly. While there, I noticed that sniblocking tests were wrong because of the previous netem behavior and asserted that the control should return a nil failure, while it should have been `ssl_invalid_hostname`. Part of ooni/probe#2531
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
As explained in ooni/netem#40, netem used to generate on the fly certificates for any host. I then discovered that this behavior is not desirable because the internet doesn't work like that. Specifically, TLS dialing to, say, www.example.com with www.google.com as the SNI must return a TLS error (`ssl_invalid_hostname` in OONI). This wrong behavior of netem has become a bottleneck for writing new tests for the beacons functionality, so it must change. I have already changed netem and this diff updates the probe-cli tree to use a netem version that behaves more correctly. While there, I noticed that sniblocking tests were wrong because of the previous netem behavior and asserted that the control should return a nil failure, while it should have been `ssl_invalid_hostname`. Part of ooni/probe#2531
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before writing this diff, netem automatically generated the correct cert on the fly based on the given SNI or local addr.
While this behavior has been okay so far, it turns out there is a set of tests we cannot write because of it.
Namely, we cannot check whether we can connect to a given host using another SNI, because netem would generate a certificate for the provided SNI, which is not how the internet works.
If I'm connecting to www.example.com with www.google.com as the SNI, in the internet the server would return a valid certificate for www.example.com, rather than for www.google.com.
This diff rectifies netem's behavior by forcing the programmer to pin the certificate to a set of names and addresses when creating the server that needs such a certificate.
Accordingly, we can stop using a forked google/martian/v3/mitm implementation and we can just roll out our own, which is still based on martian (fear not, not reinventing the wheel here!) but allows us to create a certificate with specific addresses and domain names pinned to it.
While there, notice that there was code we were not using, such as stdlib.go, and that we also don't need in ooni/probe-cli, so we can definitely kill this piece of code.
While there, don't be shy and make a bunch of constructors of the
Mustkind. They will panic on failure, which is fine in the netem context, because this library is meant to only be used when writing tests.Part of ooni/probe#2531, because the tests I was trying to write belong to such an issue.