quic: TLS handshake saver, error classification #1126

kelmenhorst · 2020-12-08T16:06:52Z

Part of #1057.

into issue/1024

…ngine into issue/1057

…into issue/1057

bassosimone · 2021-01-07T12:00:45Z

👀

bassosimone

Thank you so much! This PR contains terrific work!

My review:

provides many small style-related suggestions;
asks questions regarding the split between QUIC and normal errors;
asks whether it's possible to have all savers in the same place.

🎸

bassosimone · 2021-01-07T16:45:43Z

netx/dialer/quic_connectionstate_go1.14.go

+)
+
+// ConnectionState returns an empty connection state
+// because the quic-go interface go 1.15


This sentence here seems incomplete?

bassosimone · 2021-01-07T16:46:18Z

netx/dialer/quic_connectionstate_go1.15.go

+	"github.com/lucas-clemente/quic-go"
+)
+
+// ConnectionState returns the connection state of a session which is only accessible using go 1.15


I think it's fine to avoid mentioning Go 1.15 explicitly here

bassosimone · 2021-01-07T16:46:58Z

netx/dialer/quic_dialer.go

+	"github.com/ooni/probe-engine/netx/trace"
+)
+
+// QUICBaseDialer is a system dialer for Quic


Suggested change

// QUICBaseDialer is a system dialer for Quic

// QUICBaseDialer is a system dialer for QUIC

bassosimone · 2021-01-07T16:47:38Z

netx/dialer/quic_dialer.go

+	DialContext(ctx context.Context, network, addr string, host string, tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlySession, error)
+}
+
+// QUICDialer is the definition of dialer for QUIC assumed by this package.


This documentation comment should be updated because we have several dialers now

bassosimone · 2021-01-07T16:48:01Z

netx/dialer/quic_dialer.go

+	Dial(network, addr string, tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlySession, error)
+}
+
+// QUICSystemDialer is a base dialer for QUIC


Suggested change

// QUICSystemDialer is a base dialer for QUIC

// QUICSystemDialer is the base dialer for QUIC

bassosimone · 2021-01-07T17:12:28Z

netx/dialer/quicsaver_test.go

+
+func (d MockQUICDialer) DialContext(ctx context.Context, network, addr string, host string, tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlySession, error) {
+	if d.Dialer != nil {
+		d.Dialer.DialContext(ctx, network, addr, host, tlsCfg, cfg)


I find it surprising that you don't return here.

bassosimone · 2021-01-07T17:15:23Z

netx/dialer/quicsaver_test.go

+	if ev[1].Err != nil {
+		t.Fatal("unexpected Err", ev[1].Err)
+	}
+	if ev[1].Name != "quic_handshake_done" {


Yeah, these strings maybe should just be defined once in errorx

I did this in correspondence to saver_test.go , seeing that, for instance, "tls_handshake_done" is also not defined in errorx.

Yes, I've just checked saver.go. It seems only the tls_handshake_{start,done} events do not have a corresponding name inside of errorx.go. I think I understand what is going on.

We have operations, whose name is defined in errorx.go and then we have events. Some events have the same name of operations, so we use the names in errorx while some others do not. This is the case of "tls_handshake_done".

It may be useful to have all these names in the same place to avoid typing errors, nonetheless. But maybe we can do this at a later time.

yes, if so, it should be changed for TCP and QUIC both, I think

I agree with you. This does not seem a blocker to me. Let's do nothing for now. The code is already consistent as is.

bassosimone · 2021-01-07T17:17:58Z

netx/errorx/errorx.go

+	failureString := toFailureString
+	if b.QuicErr {
+		failureString = toQUICFailureString
+	}


We should chat about this part of the design. I would have instinctively put checks for QUIC related errors into the function we're already using for checking for errors. Maybe I'm missing something?

Yes, this would probably be more clear. In doing that, should, for example, a timeout error, induce the same notation in the result file, regardless whether it happened during a QUIC or a TCP connection? Or do you prefer having a generic_timeout_error and a quic_generic_timeout_error?

I would keep a generic_timeout_error for now. I think maybe in the future we'll need to have more specific or different errors. But, for now, it seems to me we understand what is going on by combining operation and error.

(The error is generic_timeout_error rather than just timeout or timeout_error for legacy reasons that I honestly do not remember. Every time I write a table in LaTeX, I regret that this name is so long.)

So I moved the QUIC specific content from the toQUICFailureString function into the toFailureString function and updated the tests. I added the QUIC specific failures in a batch at the end of the function. I'm not sure if the order should instead be based on the failure type.

netx/httptransport/http3transport.go

bassosimone · 2021-01-07T17:20:20Z

netx/netx.go

 	if config.FullResolver == nil {
 		config.FullResolver = NewResolver(config)
 	}
-	d := &dialer.HTTP3DNSDialer{Resolver: config.FullResolver}
-	var dialer HTTP3Dialer = &httptransport.HTTP3WrapperDialer{Dialer: d}
+	var d dialer.QUICContextDialer = &dialer.QUICSystemDialer{Saver: config.ReadWriteSaver}


I also mentioned above that I don't 100% understand this saver here. Maybe I'm missing something?

Update: I think now I understand its purpose. Please refer to this comment: #1126 (comment).

Do you think the ReadWriteSaver should be encapsulated in a higher-level Dialer? In this case I'd have to provide access to the UDPconn struct to higher level Dialers.

I'll ask you more details in chat. This part of the convo seems complex.

So, for the records. That is the only place where we can observe a UDP connection and wrap it. Doing this kind of wrapping elsewhere is possible, but seems also less practical.

Avoid names like quicdialer.QUICFoo. We're already into the quicdialer package, so we can save typing the QUIC prefix. Wrap lines that seem to long (a subjective call, of course). It seems there were two similar mockable dialers, so unify them such that we only have one. We gain a little in simplicity.

A couple of cosmetic changes. Removed likely redundant tests. Note that we still need to implement one more test here.

bassosimone

I've read this diff very carefully, checked out the code myself, run tests, added formatting changes, and other minor changes to this pull request.

The code in this pull request is excellent! I am really looking forward to merging this code and having enhanced QUIC measurement capabilities.

Thank you so much, you did a great job! I'm going to merge as soon as checks are green!

🐳

kelmenhorst and others added 30 commits November 8, 2020 20:08

dns dialer for http3 transport

c3782f1

added HTTP3Dialer to http3transport test

f3d83e6

use config to pass dialers to http factory

3690e00

explicitly initialising fields

bb3c22e

more explicit comment

66ae1a1

changed name NewHTTP3Dialer

1852147

added forgotten TODO comment back in

a806201

removed inconsistent error return statements

1d968bd

added more integration tests

5170882

close UDP connection

e20af4a

improved error handling, added respective test

e3042e4

test DialEarly error path

a7c9f00

use errors.Is pattern

f178790

fix: make sure tests pass on macOS

8f9acaa

Merge remote-tracking branch 'origin/master' into pr/1029

b54e78b

Update netx/dialer/http3dns.go

3dfb809

Update netx/dialer/http3dns_test.go

32f9659

Update netx/httptransport/http3transport_test.go

d7eced3

Merge remote-tracking branch 'upstream/master' into issue/1024

781bf36

Merge branch 'issue/1024' of https://github.com/kelmenhorst/probe-engine

a25a6bc

into issue/1024

use context for http3 dialer

1500f24

use custom tls config for http3 transport

3f98a09

h3 qa: added tests for custom SNI and CA bundle

36d40ce

added test for unverified sni

1fc0a41

pr review 4/12: avoid casting, use tls.Config

3c4e8c4

pr review 4/12: wrapped long lines

9ecf7f8

pr review 4/12: small changes Mock HTTP3 Dialing

61841d7

pr review 4/12: removed "Unit" from test names

bf3f843

added explanation for Wrapper Dialer

2d1bbb2

Merge branch 'issue/1057_2' of https://github.com/kelmenhorst/probe-e…

aeb0631

…ngine into issue/1057

kelmenhorst marked this pull request as ready for review December 15, 2020 14:32

kelmenhorst requested review from bassosimone and hellais as code owners December 15, 2020 14:32

kelmenhorst added 4 commits January 3, 2021 16:02

Merge branch 'master' of https://github.com/kelmenhorst/probe-engine …

b0830b2

…into issue/1057

added quic_handshake for saver dialers

ca1042b

fixed quicsaver test

879af5d

removed stupid udp connection closing that caused false positives

b9cec50

preferred quic error format: prefix "quic_"

60a7d8f

bassosimone reviewed Jan 7, 2021

View reviewed changes

kelmenhorst and others added 17 commits January 7, 2021 19:21

pr review 2021/01/07: smaller changes

8e65f33

removed unused QUICBaseDialer

6b18280

pr review 2021/01/07: suggested changes in QUICSystemDialer

6051068

pr review 2021/01/07: design changes

5f10813

pr review 2021/01/07: refactored QUIC errorx

d804022

added quicdialer package, renamed its files

5591cab

exposed dialer.ReduceErrors

5c9f81c

moved PeerCerts to netx/trace

f7c4dc5

removed QUIC SaverDialer

ca2e86c

only use one host string for QUIC DialContext()

3b21732

Merge remote-tracking branch 'origin/master' into pr/1126

1b446ae

quicdialer: continue review

5a615a8

A couple of cosmetic changes. Removed likely redundant tests. Note that we still need to implement one more test here.

netx/quic: minor adjustements to comments

36cc337

mention a couple more TODOs

3769d6b

record the last todo to keep in mind

eab8f50

remove what seems to be a duplicate test

abc633c

bassosimone approved these changes Jan 11, 2021

View reviewed changes

bassosimone mentioned this pull request Jan 11, 2021

quic/h3: quality assurance (1/n) #1057

Closed

bassosimone merged commit 83d6d95 into ooni:master Jan 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: TLS handshake saver, error classification #1126

quic: TLS handshake saver, error classification #1126

kelmenhorst commented Dec 8, 2020 •

edited by bassosimone

Loading

bassosimone commented Jan 7, 2021

bassosimone left a comment

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

kelmenhorst Jan 7, 2021

bassosimone Jan 7, 2021 •

edited

Loading

kelmenhorst Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

kelmenhorst Jan 7, 2021

bassosimone Jan 7, 2021

kelmenhorst Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

kelmenhorst Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone Jan 7, 2021

bassosimone left a comment

	// QUICBaseDialer is a system dialer for Quic
	// QUICBaseDialer is a system dialer for QUIC

	// QUICSystemDialer is a base dialer for QUIC
	// QUICSystemDialer is the base dialer for QUIC

quic: TLS handshake saver, error classification #1126

quic: TLS handshake saver, error classification #1126

Conversation

kelmenhorst commented Dec 8, 2020 • edited by bassosimone Loading

bassosimone commented Jan 7, 2021

bassosimone left a 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

Choose a reason for hiding this comment

bassosimone Jan 7, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassosimone left a comment

Choose a reason for hiding this comment

kelmenhorst commented Dec 8, 2020 •

edited by bassosimone

Loading

bassosimone Jan 7, 2021 •

edited

Loading