feat: echcheck experiment

[d1vyank]

Checklist

• I have read the contribution guidelines
• reference issue for this pull request: add probe for TLSv1.3 ECH (Encrypted ClientHello, ESNI replacement) probe#1453
• if you changed anything related how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: add echcheck experiment spec spec#263

Description

Adds a new echcheck experiment that tests for interference of TLS handshakes with the Encrypted Client Hello (ECH) extension present.

[bassosimone]

🥳 I will take a look! Thank you!

[kelmenhorst]

Thank you so much @d1vyank for contributing this great ECH experiment! This is a super important issue and I think it will be very insightful to measure the blocking of (GREASE'd) ECH. 🥳

I carefully read the code and the relevant chapters of the reference IETF draft, and tested it locally. Overall, I think the experiment is really good! I suggested some minor changes which are mostly related to typos and the tests. One improvement to consider would be to run the target and control handshake concurrently to avoid bias caused by orderly execution.

Thanks again! 🙏

[kelmenhorst]

I wonder whether the subsequent handshakes for control and target should be executed in parallel using go routines such that we avoid bias caused by orderly execution. What do you think?

[d1vyank]

Fixed in 30e6aba.

[kelmenhorst]

Thanks for changing the code so that the handshakes are executed in go routines!
Regarding the pattern you used: Did you think about data races in this case? IIUC, you are using the waitgroup wg to make sure that both handshakes finish before the result is read, is that correct?
I am asking because I think that it would probably be a bit cleaner to use channels instead but I want to make sure that I understand what you had in mind :)

[d1vyank]

I do not see any potential for a data race here. The data shared between the go routines (start time, address and sni) is read-only and the return values are written into separate variables.

I am have trouble coming up with a cleaner way to do this using channels. Could you please elaborate on what you had in mind?

[bassosimone]

I'll chime in for a second to comment on this specific point:

I do not see any potential for a data race here. The data shared between the go routines (start time, address and sni) is read-only

Yes, because they're read only there's no data race for the input variables.

[...] and the return values are written into separate variables.

I am not convinced that the above argument is sufficient to say you don't have a data race. IIUC, you also need to guarantee that reading the return variables, which you do on line ~109 when assembling TestKeys happens after writing them in the goroutines. AFAICT, your code satisfies this happens after property. Your defer wg.Done() happens after writing the variables (by construction) and is guaranteed to happen before wg.Wait() because of how the sync.WaitGroup primitive works for synchronizing goroutines. In turn, your initialization of them happens after wg.Wait() (again by construction). I assume you omitted saying all of this for simplicity and you would agree with this analysis. However, in case I am wrong, I would like learn more about how you reasoned here.

I am have trouble coming up with a cleaner way to do this using channels. Could you please elaborate on what you had in mind?

If the above analysis on the data race safety of your code is correct, your code is like this:

var a someType

wg.Add(1)
go func() {
  defer wg.Done()
  a = foo()
}

// HERE ....

wg.Wait()
b := a

As we said it's data-races free but there is an underlying problem. It's tricky to modify. If someone by mistake moves the assignment in the area marked as HERE you would now have a data race. I know this should not happen in theory, though there are many mistakes one could do with refactoring and this is still one of them. (We would probably catch this issue because we run tests with -race but that's beside the point.)

Compare the code above with this code:

ach := make(chan someType)

go func() {
  ach <- foo()
}

b := <-ach

This pattern has the same data-race-safe properties of the code above and additionally it is impossible to refactor it in a way that the memory barrier provided by wg.Wait is decoupled from accessing the variable, because reading a channel is a memory barrier and a way to extract information from the goroutine as part of the same operation.

In your case, the code would probably look like this code:

ach1 := make(chan someType)
ach2 := make(chan someType)

go func() {
  ach1 <- foo()
}

go func() {
  ach2 <- bar()
}

b := <-ach1
c := <-ach2

Because you need anyway to wait for both goroutines to finish running, it does not matter much the relative order with which you read b and c. If ach2 is written before ach1, you just wait for ach1 to be written as well and then the read operation on the ach2 channel would not block. Regardless, once you've read both channels, you have values of b and c that were obtained with a synchronization primitive that combines synchronization and message passing and as such when reading the code you don't need to worry about the order of locking and reading.

[d1vyank]

Thank you for the detailed explanation. I agree that the WaitGroup implementation is error prone as it depends on the order of blocking and reading.

I have implemented the channel pattern you suggested in c89d51c.

[d1vyank]

Thank you, @kelmenhorst for taking the time to review this and giving your feedback. I've addressed and responded to your comments individually. Please take a look!

[kelmenhorst]

Thank you so much @d1vyank for contributing this experiment and for implementing the requested changes! It looks really good and I will merge now 💯 🎉