feat: save CNAME into archival data format (#877)

* feat: save CNAME into archival data format

When a DNSResponse contains a non-empty CNAME, we include it
into the related list of answers.

Closes https://github.com/ooni/probe/issues/2227

* doc: add design note

While there, make code more compact and robust to a case where
we're going to extract additional answers.

* doc: document the expected growth of extraction function

Based on feedback by @DecFox

internal/measurexlite/dns.go

@@ -114,7 +114,7 @@ type DNSNetworkAddresser interface {
 func NewArchivalDNSLookupResultFromRoundTrip(index int64, started time.Duration, reso DNSNetworkAddresser, query model.DNSQuery,
 	response model.DNSResponse, addrs []string, err error, finished time.Duration) *model.ArchivalDNSLookupResult {
 	return &model.ArchivalDNSLookupResult{
-		Answers:          archivalAnswersFromAddrs(addrs),
+		Answers:          newArchivalDNSAnswers(addrs, response),
 		Engine:           reso.Network(),
 		Failure:          tracex.NewFailure(err),
 		Hostname:         query.Domain(),
@@ -125,8 +125,16 @@ func NewArchivalDNSLookupResultFromRoundTrip(index int64, started time.Duration,
 	}
 }
 
-// archivalAnswersFromAddrs generates model.ArchivalDNSAnswer from an array of addresses
+// newArchivalDNSAnswers generates []model.ArchivalDNSAnswer from [addrs] and [resp].
-func archivalAnswersFromAddrs(addrs []string) (out []model.ArchivalDNSAnswer) {
+func newArchivalDNSAnswers(addrs []string, resp model.DNSResponse) (out []model.ArchivalDNSAnswer) {
+	// Design note: in principle we might want to extract everything from the
+	// response but, when we're called by netxlite, netxlite has already extracted
+	// the addresses to return them to the caller, so I think it's fine to keep
+	// this extraction code as such rather than suppressing passing the addrs from
+	// netxlite. Also, a wrong IP address is a bug because netxlite should not
+	// return invalid IP addresses from its resolvers, so we want to know about that.
+
+	// Include IP addresses extracted by netxlite
 	for _, addr := range addrs {
 		ipv6, err := netxlite.IsIPv6(addr)
 		if err != nil {
@@ -142,6 +150,7 @@ func archivalAnswersFromAddrs(addrs []string) (out []model.ArchivalDNSAnswer) {
 				AnswerType: "A",
 				Hostname:   "",
 				IPv4:       addr,
+				IPv6:       "",
 				TTL:        nil,
 			})
 		case true:
@@ -150,11 +159,32 @@ func archivalAnswersFromAddrs(addrs []string) (out []model.ArchivalDNSAnswer) {
 				ASOrgName:  org,
 				AnswerType: "AAAA",
 				Hostname:   "",
+				IPv4:       "",
 				IPv6:       addr,
 				TTL:        nil,
 			})
 		}
 	}
+
+	// Include additional answer types when a response is available
+	if resp != nil {
+
+		// Include CNAME if available
+		if cname, err := resp.DecodeCNAME(); err == nil && cname != "" {
+			out = append(out, model.ArchivalDNSAnswer{
+				ASN:        0,
+				ASOrgName:  "",
+				AnswerType: "CNAME",
+				Hostname:   cname,
+				IPv4:       "",
+				IPv6:       "",
+				TTL:        nil,
+			})
+		}
+
+		// TODO(bassosimone): what other fields generally present inside A/AAAA replies
+		// would it be useful to extract here? Perhaps, the SoA field?
+	}
 	return
 }
 

internal/measurexlite/dns_test.go

@@ -135,6 +135,9 @@ func TestNewResolver(t *testing.T) {
 						}
 						return []string{"1.1.1.1"}, nil
 					},
+					MockDecodeCNAME: func() (string, error) {
+						return "dns.google.", nil
+					},
 				}
 				return response, nil
 			},
@@ -177,7 +180,7 @@ func TestNewResolver(t *testing.T) {
 				if ev.Engine != "mocked" {
 					t.Fatal("unexpected engine")
 				}
-				if len(ev.Answers) != 1 {
+				if len(ev.Answers) != 2 {
 					t.Fatal("expected single answer in DNSLookup event")
 				}
 				if ev.QueryType == "A" && ev.Answers[0].IPv4 != "1.1.1.1" {
@@ -186,6 +189,9 @@ func TestNewResolver(t *testing.T) {
 				if ev.QueryType == "AAAA" && ev.Answers[0].IPv6 != "fe80::a00:20ff:feb9:4c54" {
 					t.Fatal("unexpected AAAA query result")
 				}
+				if ev.Answers[1].AnswerType != "CNAME " && ev.Answers[1].Hostname != "dns.google." {
+					t.Fatal("unexpected second answer (expected CNAME)", ev.Answers[1])
+				}
 			}
 		})
 	})
@@ -205,6 +211,9 @@ func TestNewResolver(t *testing.T) {
 						}
 						return []string{"1.1.1.1"}, nil
 					},
+					MockDecodeCNAME: func() (string, error) {
+						return "dns.google.", nil
+					},
 				}
 				return response, nil
 			},
@@ -350,8 +359,12 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) {
 			addrs := []string{"1.1.1.1"}
 			finished := trace.TimeNow()
 			// 1. fill the trace
-			err := trace.OnDelayedDNSResponse(started, txp, query, &mocks.DNSResponse{},
+			dnsResponse := &mocks.DNSResponse{
-				addrs, nil, finished)
+				MockDecodeCNAME: func() (string, error) {
+					return "", netxlite.ErrOODNSNoAnswer
+				},
+			}
+			err := trace.OnDelayedDNSResponse(started, txp, query, dnsResponse, addrs, nil, finished)
 			// 2. read the trace
 			got := trace.DelayedDNSResponseWithTimeout(context.Background(), time.Second)
 			if err != nil {
@@ -388,8 +401,12 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) {
 			addrs := []string{"1.1.1.1"}
 			finished := trace.TimeNow()
 			// 1. attempt to write into the trace
-			err := trace.OnDelayedDNSResponse(started, txp, query, &mocks.DNSResponse{},
+			dnsResponse := &mocks.DNSResponse{
-				addrs, nil, finished)
+				MockDecodeCNAME: func() (string, error) {
+					return "", netxlite.ErrOODNSNoAnswer
+				},
+			}
+			err := trace.OnDelayedDNSResponse(started, txp, query, dnsResponse, addrs, nil, finished)
 			if !errors.Is(err, ErrDelayedDNSResponseBufferFull) {
 				t.Fatal("unexpected error", err)
 			}
@@ -427,10 +444,15 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) {
 			addrs := []string{"1.1.1.1"}
 			finished := trace.TimeNow()
 			events := 4
+			dnsResponse := &mocks.DNSResponse{
+				MockDecodeCNAME: func() (string, error) {
+					return "", netxlite.ErrOODNSNoAnswer
+				},
+			}
 			for i := 0; i < events; i++ {
 				// fill the trace
 				trace.delayedDNSResponse <- NewArchivalDNSLookupResultFromRoundTrip(trace.Index, started.Sub(trace.ZeroTime),
-					txp, query, &mocks.DNSResponse{}, addrs, nil, finished.Sub(trace.ZeroTime))
+					txp, query, dnsResponse, addrs, nil, finished.Sub(trace.ZeroTime))
 			}
 			ctx, cancel := context.WithCancel(context.Background())
 			cancel() // we ensure that the context cancels before draining all the events
@@ -465,8 +487,13 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) {
 			}
 			addrs := []string{"1.1.1.1"}
 			finished := trace.TimeNow()
+			dnsResponse := &mocks.DNSResponse{
+				MockDecodeCNAME: func() (string, error) {
+					return "", netxlite.ErrOODNSNoAnswer
+				},
+			}
 			trace.delayedDNSResponse <- NewArchivalDNSLookupResultFromRoundTrip(trace.Index, started.Sub(trace.ZeroTime),
-				txp, query, &mocks.DNSResponse{}, addrs, nil, finished.Sub(trace.ZeroTime))
+				txp, query, dnsResponse, addrs, nil, finished.Sub(trace.ZeroTime))
 			got := trace.DelayedDNSResponseWithTimeout(context.Background(), time.Second)
 			if len(got) != 1 {
 				t.Fatal("unexpected output from trace")
@@ -475,52 +502,156 @@ func TestDelayedDNSResponseWithTimeout(t *testing.T) {
 	})
 }
 
-func TestAnswersFromAddrs(t *testing.T) {
+func TestNewArchivalDNSAnswers(t *testing.T) {
 	tests := []struct {
 		name     string
-		args []string
+		addrs    []string
+		resp     model.DNSResponse
+		expected []model.ArchivalDNSAnswer
 	}{{
 		name: "with valid input",
-		args: []string{"1.1.1.1", "fe80::a00:20ff:feb9:4c54"},
+		addrs: []string{
+			"8.8.4.4",
+			"2001:4860:4860::8844",
+		},
+		resp: nil,
+		expected: []model.ArchivalDNSAnswer{{
+			ASN:        15169,
+			ASOrgName:  "Google LLC",
+			AnswerType: "A",
+			Hostname:   "",
+			IPv4:       "8.8.4.4",
+			IPv6:       "",
+			TTL:        nil,
+		}, {
+			ASN:        15169,
+			ASOrgName:  "Google LLC",
+			AnswerType: "AAAA",
+			Hostname:   "",
+			IPv4:       "",
+			IPv6:       "2001:4860:4860::8844",
+			TTL:        nil,
+		}},
 	}, {
 		name: "with invalid IPv4 address",
-		args: []string{"1.1.1.1.1", "fe80::a00:20ff:feb9:4c54"},
+		addrs: []string{
+			"1.1.1.1.1", // invalid because it has five dots
+			"2001:4860:4860::8844",
+		},
+		resp: nil,
+		expected: []model.ArchivalDNSAnswer{{
+			ASN:        15169,
+			ASOrgName:  "Google LLC",
+			AnswerType: "AAAA",
+			Hostname:   "",
+			IPv4:       "",
+			IPv6:       "2001:4860:4860::8844",
+			TTL:        nil,
+		}},
 	}, {
 		name: "with invalid IPv6 address",
-		args: []string{"1.1.1.1", "fe80::a00:20ff:feb9:::4c54"},
+		addrs: []string{
+			"8.8.4.4",
+			"fe80::a00:20ff:feb9:::4c54", // invalid because it has :::
+		},
+		resp: nil,
+		expected: []model.ArchivalDNSAnswer{{
+			ASN:        15169,
+			ASOrgName:  "Google LLC",
+			AnswerType: "A",
+			Hostname:   "",
+			IPv4:       "8.8.4.4",
+			IPv6:       "",
+			TTL:        nil,
+		}},
 	}, {
 		name:     "with empty input",
-		args: []string{},
+		addrs:    []string{},
+		resp:     nil,
+		expected: nil,
 	}, {
 		name:     "with nil input",
-		args: nil,
+		addrs:    nil,
+		resp:     nil,
+		expected: nil,
+	}, {
+		name: "with valid IPv4 address and CNAME",
+		addrs: []string{
+			"8.8.8.8",
+		},
+		resp: &mocks.DNSResponse{
+			MockDecodeCNAME: func() (string, error) {
+				return "dns.google.", nil
+			},
+		},
+		expected: []model.ArchivalDNSAnswer{{
+			ASN:        15169,
+			ASOrgName:  "Google LLC",
+			AnswerType: "A",
+			Hostname:   "",
+			IPv4:       "8.8.8.8",
+			IPv6:       "",
+			TTL:        nil,
+		}, {
+			ASN:        0,
+			ASOrgName:  "",
+			AnswerType: "CNAME",
+			Hostname:   "dns.google.",
+			IPv4:       "",
+			IPv6:       "",
+			TTL:        nil,
+		}},
+	}, {
+		name: "with valid IPv6 address and CNAME",
+		addrs: []string{
+			"2001:4860:4860::8844",
+		},
+		resp: &mocks.DNSResponse{
+			MockDecodeCNAME: func() (string, error) {
+				return "dns.google.", nil
+			},
+		},
+		expected: []model.ArchivalDNSAnswer{{
+			ASN:        15169,
+			ASOrgName:  "Google LLC",
+			AnswerType: "AAAA",
+			Hostname:   "",
+			IPv4:       "",
+			IPv6:       "2001:4860:4860::8844",
+			TTL:        nil,
+		}, {
+			ASN:        0,
+			ASOrgName:  "",
+			AnswerType: "CNAME",
+			Hostname:   "dns.google.",
+			IPv4:       "",
+			IPv6:       "",
+			TTL:        nil,
+		}},
+	}, {
+		name:  "with DecodeCNAME error",
+		addrs: []string{},
+		resp: &mocks.DNSResponse{
+			MockDecodeCNAME: func() (string, error) {
+				return "", errors.New("mocked errorr")
+			},
+		},
+		expected: nil,
+	}, {
+		name:  "with DecodeCNAME success and no CNAME",
+		addrs: []string{},
+		resp: &mocks.DNSResponse{
+			MockDecodeCNAME: func() (string, error) {
+				return "", nil
+			},
+		},
+		expected: nil,
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := archivalAnswersFromAddrs(tt.args)
+			got := newArchivalDNSAnswers(tt.addrs, tt.resp)
-			var idx int
+			if diff := cmp.Diff(tt.expected, got); diff != "" {
-			for _, inp := range tt.args {
+				t.Fatal(diff)
-				ip6, err := netxlite.IsIPv6(inp)
-				if err != nil {
-					continue
-				}
-				if idx >= len(got) {
-					t.Fatal("unexpected array length")
-				}
-				answer := got[idx]
-				if ip6 {
-					if answer.AnswerType != "AAAA" || answer.IPv6 != inp {
-						t.Fatal("unexpected output", answer)
-					}
-				} else {
-					if answer.AnswerType != "A" || answer.IPv4 != inp {
-						t.Fatal("unexpected output", answer)
-					}
-				}
-				idx++
-			}
-			if idx != len(got) {
-				t.Fatal("unexpected array length", len(got))
 			}
 		})
 	}