From 9354191b85008f07f6ba23a530b5ef73bd59c542 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 2 Jun 2022 11:07:02 +0200 Subject: [PATCH] refactor(tracex): internally store just the raw certificate (#787) By just storing the raw certificate we simplify the internal data structure we use. In turn, this enables us to write better unit tests using github.com/google/go-cmp where we can construct the expected result and compare with that. (Yeah, in principle we could also construct the full certificate but I'm not sure it's worth the effort since we basically only care about the raw certificate.) The general idea here is to make tracex more tested. Once it's more tested, I will create separate structs for each event, which is something that measurex also does. Once that is done, we can start ensuring that the code in measurex and the code in tracex do the same thing in terms of storing observations. When also this is done, we can then rewrite measurex to use tracex directly. The overall goal is https://github.com/ooni/probe/issues/2035. --- internal/tracex/archival.go | 5 ++- internal/tracex/archival_test.go | 19 +++++------- internal/tracex/event.go | 53 ++++++++++++++++---------------- internal/tracex/quic.go | 1 + internal/tracex/quic_test.go | 6 ++-- internal/tracex/tls.go | 13 +++++--- internal/tracex/tls_test.go | 18 ++++++----- 7 files changed, 59 insertions(+), 56 deletions(-) diff --git a/internal/tracex/archival.go b/internal/tracex/archival.go index 6f95bb2..0351c42 100644 --- a/internal/tracex/archival.go +++ b/internal/tracex/archival.go @@ -5,7 +5,6 @@ package tracex // import ( - "crypto/x509" "errors" "net" "net/http" @@ -303,9 +302,9 @@ func NewTLSHandshakesList(begin time.Time, events []Event) (out []TLSHandshake) return } -func tlsMakePeerCerts(in []*x509.Certificate) (out []MaybeBinaryValue) { +func tlsMakePeerCerts(in [][]byte) (out []MaybeBinaryValue) { for _, entry := range in { - out = append(out, MaybeBinaryValue{Value: string(entry.Raw)}) + out = append(out, MaybeBinaryValue{Value: string(entry)}) } return } diff --git a/internal/tracex/archival_test.go b/internal/tracex/archival_test.go index 479b32f..756e3eb 100644 --- a/internal/tracex/archival_test.go +++ b/internal/tracex/archival_test.go @@ -2,7 +2,6 @@ package tracex import ( "context" - "crypto/x509" "errors" "io" "net/http" @@ -533,11 +532,10 @@ func TestNewTLSHandshakesList(t *testing.T) { Proto: "tcp", TLSCipherSuite: "SUITE", TLSNegotiatedProto: "h2", - TLSPeerCerts: []*x509.Certificate{{ - Raw: []byte("deadbeef"), - }, { - Raw: []byte("abad1dea"), - }}, + TLSPeerCerts: [][]byte{ + []byte("deadbeef"), + []byte("abad1dea"), + }, TLSServerName: "x.org", TLSVersion: "TLSv1.3", Time: begin.Add(55 * time.Millisecond), @@ -569,11 +567,10 @@ func TestNewTLSHandshakesList(t *testing.T) { Proto: "quic", TLSCipherSuite: "SUITE", TLSNegotiatedProto: "h3", - TLSPeerCerts: []*x509.Certificate{{ - Raw: []byte("deadbeef"), - }, { - Raw: []byte("abad1dea"), - }}, + TLSPeerCerts: [][]byte{ + []byte("deadbeef"), + []byte("abad1dea"), + }, TLSServerName: "x.org", TLSVersion: "TLSv1.3", Time: begin.Add(55 * time.Millisecond), diff --git a/internal/tracex/event.go b/internal/tracex/event.go index 13c43b4..76a1d0e 100644 --- a/internal/tracex/event.go +++ b/internal/tracex/event.go @@ -5,7 +5,6 @@ package tracex // import ( - "crypto/x509" "errors" "net/http" "time" @@ -275,30 +274,30 @@ func (ev *EventWriteOperation) Name() string { // Event is one of the events within a trace type EventValue struct { - Addresses []string `json:",omitempty"` - Address string `json:",omitempty"` - DNSQuery []byte `json:",omitempty"` - DNSResponse []byte `json:",omitempty"` - Data []byte `json:",omitempty"` - Duration time.Duration `json:",omitempty"` - Err FailureStr `json:",omitempty"` - HTTPMethod string `json:",omitempty"` - HTTPRequestHeaders http.Header `json:",omitempty"` - HTTPResponseHeaders http.Header `json:",omitempty"` - HTTPResponseBody []byte `json:",omitempty"` - HTTPResponseBodyIsTruncated bool `json:",omitempty"` - HTTPStatusCode int `json:",omitempty"` - HTTPURL string `json:",omitempty"` - Hostname string `json:",omitempty"` - NoTLSVerify bool `json:",omitempty"` - NumBytes int `json:",omitempty"` - Proto string `json:",omitempty"` - TLSServerName string `json:",omitempty"` - TLSCipherSuite string `json:",omitempty"` - TLSNegotiatedProto string `json:",omitempty"` - TLSNextProtos []string `json:",omitempty"` - TLSPeerCerts []*x509.Certificate `json:",omitempty"` - TLSVersion string `json:",omitempty"` - Time time.Time `json:",omitempty"` - Transport string `json:",omitempty"` + Addresses []string `json:",omitempty"` + Address string `json:",omitempty"` + DNSQuery []byte `json:",omitempty"` + DNSResponse []byte `json:",omitempty"` + Data []byte `json:",omitempty"` + Duration time.Duration `json:",omitempty"` + Err FailureStr `json:",omitempty"` + HTTPMethod string `json:",omitempty"` + HTTPRequestHeaders http.Header `json:",omitempty"` + HTTPResponseHeaders http.Header `json:",omitempty"` + HTTPResponseBody []byte `json:",omitempty"` + HTTPResponseBodyIsTruncated bool `json:",omitempty"` + HTTPStatusCode int `json:",omitempty"` + HTTPURL string `json:",omitempty"` + Hostname string `json:",omitempty"` + NoTLSVerify bool `json:",omitempty"` + NumBytes int `json:",omitempty"` + Proto string `json:",omitempty"` + TLSServerName string `json:",omitempty"` + TLSCipherSuite string `json:",omitempty"` + TLSNegotiatedProto string `json:",omitempty"` + TLSNextProtos []string `json:",omitempty"` + TLSPeerCerts [][]byte `json:",omitempty"` + TLSVersion string `json:",omitempty"` + Time time.Time `json:",omitempty"` + Transport string `json:",omitempty"` } diff --git a/internal/tracex/quic.go b/internal/tracex/quic.go index 0d9882e..dfea764 100644 --- a/internal/tracex/quic.go +++ b/internal/tracex/quic.go @@ -64,6 +64,7 @@ func (h *QUICDialerSaver) DialContext(ctx context.Context, network string, NoTLSVerify: tlsCfg.InsecureSkipVerify, Proto: network, TLSNextProtos: tlsCfg.NextProtos, + TLSPeerCerts: [][]byte{}, TLSServerName: tlsCfg.ServerName, Time: stop, }}) diff --git a/internal/tracex/quic_test.go b/internal/tracex/quic_test.go index 8e54b0b..7a4aac9 100644 --- a/internal/tracex/quic_test.go +++ b/internal/tracex/quic_test.go @@ -59,7 +59,7 @@ func TestQUICDialerSaver(t *testing.T) { if value.TLSNegotiatedProto != "h3" { t.Fatal("invalid negotiated protocol") } - if diff := cmp.Diff(value.TLSPeerCerts, []*x509.Certificate{}); diff != "" { + if diff := cmp.Diff(value.TLSPeerCerts, [][]byte{{1, 2, 3, 4}}); diff != "" { t.Fatal(diff) } if value.TLSVersion != "TLSv1.3" { @@ -83,7 +83,9 @@ func TestQUICDialerSaver(t *testing.T) { cs := quic.ConnectionState{} cs.TLS.ConnectionState.CipherSuite = tls.TLS_RSA_WITH_RC4_128_SHA cs.TLS.NegotiatedProtocol = "h3" - cs.TLS.PeerCertificates = []*x509.Certificate{} + cs.TLS.PeerCertificates = []*x509.Certificate{{ + Raw: []byte{1, 2, 3, 4}, + }} cs.TLS.Version = tls.VersionTLS13 return cs }, diff --git a/internal/tracex/tls.go b/internal/tracex/tls.go index 128b27d..c7659de 100644 --- a/internal/tracex/tls.go +++ b/internal/tracex/tls.go @@ -77,22 +77,25 @@ var _ model.TLSHandshaker = &TLSHandshakerSaver{} // tlsPeerCerts returns the certificates presented by the peer regardless // of whether the TLS handshake was successful -func tlsPeerCerts(state tls.ConnectionState, err error) []*x509.Certificate { +func tlsPeerCerts(state tls.ConnectionState, err error) (out [][]byte) { var x509HostnameError x509.HostnameError if errors.As(err, &x509HostnameError) { // Test case: https://wrong.host.badssl.com/ - return []*x509.Certificate{x509HostnameError.Certificate} + return [][]byte{x509HostnameError.Certificate.Raw} } var x509UnknownAuthorityError x509.UnknownAuthorityError if errors.As(err, &x509UnknownAuthorityError) { // Test case: https://self-signed.badssl.com/. This error has // never been among the ones returned by MK. - return []*x509.Certificate{x509UnknownAuthorityError.Cert} + return [][]byte{x509UnknownAuthorityError.Cert.Raw} } var x509CertificateInvalidError x509.CertificateInvalidError if errors.As(err, &x509CertificateInvalidError) { // Test case: https://expired.badssl.com/ - return []*x509.Certificate{x509CertificateInvalidError.Cert} + return [][]byte{x509CertificateInvalidError.Cert.Raw} } - return state.PeerCertificates + for _, cert := range state.PeerCertificates { + out = append(out, cert.Raw) + } + return } diff --git a/internal/tracex/tls_test.go b/internal/tracex/tls_test.go index 86f0319..76b36d4 100644 --- a/internal/tracex/tls_test.go +++ b/internal/tracex/tls_test.go @@ -57,7 +57,7 @@ func TestTLSHandshakerSaver(t *testing.T) { if value.TLSNegotiatedProto != "h2" { t.Fatal("invalid negotiated protocol") } - if diff := cmp.Diff(value.TLSPeerCerts, []*x509.Certificate{}); diff != "" { + if diff := cmp.Diff(value.TLSPeerCerts, [][]byte{{1, 2, 3, 4}}); diff != "" { t.Fatal(diff) } if value.TLSVersion != "TLSv1.3" { @@ -79,8 +79,10 @@ func TestTLSHandshakerSaver(t *testing.T) { returnedConnState := tls.ConnectionState{ CipherSuite: tls.TLS_RSA_WITH_RC4_128_SHA, NegotiatedProtocol: "h2", - PeerCertificates: []*x509.Certificate{}, - Version: tls.VersionTLS13, + PeerCertificates: []*x509.Certificate{{ + Raw: []byte{1, 2, 3, 4}, + }}, + Version: tls.VersionTLS13, } returnedConn := &mocks.TLSConn{ MockConnectionState: func() tls.ConnectionState { @@ -200,7 +202,7 @@ func Test_tlsPeerCerts(t *testing.T) { tests := []struct { name string args args - want []*x509.Certificate + want [][]byte }{{ name: "no error", args: args{ @@ -208,7 +210,7 @@ func Test_tlsPeerCerts(t *testing.T) { PeerCertificates: []*x509.Certificate{cert0}, }, }, - want: []*x509.Certificate{cert0}, + want: [][]byte{cert0.Raw}, }, { name: "all empty", args: args{}, @@ -221,7 +223,7 @@ func Test_tlsPeerCerts(t *testing.T) { Certificate: cert0, }, }, - want: []*x509.Certificate{cert0}, + want: [][]byte{cert0.Raw}, }, { name: "x509.UnknownAuthorityError", args: args{ @@ -230,7 +232,7 @@ func Test_tlsPeerCerts(t *testing.T) { Cert: cert0, }, }, - want: []*x509.Certificate{cert0}, + want: [][]byte{cert0.Raw}, }, { name: "x509.CertificateInvalidError", args: args{ @@ -239,7 +241,7 @@ func Test_tlsPeerCerts(t *testing.T) { Cert: cert0, }, }, - want: []*x509.Certificate{cert0}, + want: [][]byte{cert0.Raw}, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {