Skip to content

Commit

Permalink
feat: save CNAME into archival data format (#877)
Browse files Browse the repository at this point in the history 
* 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 ooni/probe#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
  • Loading branch information
@bassosimone
bassosimone authored Aug 23, 2022
1 parent 080abf9 commit 60b7d1f
Show file tree
Hide file tree
Showing 2 changed files with 205 additions and 44 deletions.
36 changes: 33 additions & 3 deletions internal/measurexlite/dns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -125,8 +125,16 @@ func NewArchivalDNSLookupResultFromRoundTrip(index int64, started time.Duration,
}
}

// archivalAnswersFromAddrs generates model.ArchivalDNSAnswer from an array of addresses
func archivalAnswersFromAddrs(addrs []string) (out []model.ArchivalDNSAnswer) {
// newArchivalDNSAnswers generates []model.ArchivalDNSAnswer from [addrs] and [resp].
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 {
Expand All @@ -142,6 +150,7 @@ func archivalAnswersFromAddrs(addrs []string) (out []model.ArchivalDNSAnswer) {
AnswerType: "A",
Hostname: "",
IPv4: addr,
IPv6: "",
TTL: nil,
})
case true:
Expand All @@ -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
}

Expand Down
213 changes: 172 additions & 41 deletions internal/measurexlite/dns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
Expand Down Expand Up @@ -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" {
Expand All @@ -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])
}
}
})
})
Expand All @@ -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
},
Expand Down Expand Up @@ -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{},
addrs, nil, finished)
dnsResponse := &mocks.DNSResponse{
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 {
Expand Down Expand Up @@ -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{},
addrs, nil, finished)
dnsResponse := &mocks.DNSResponse{
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)
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand All @@ -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
name 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",
addrs: []string{},
resp: nil,
expected: nil,
}, {
name: "with empty input",
args: []string{},
name: "with nil input",
addrs: nil,
resp: nil,
expected: nil,
}, {
name: "with nil input",
args: 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)
var idx int
for _, inp := range tt.args {
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))
got := newArchivalDNSAnswers(tt.addrs, tt.resp)
if diff := cmp.Diff(tt.expected, got); diff != "" {
t.Fatal(diff)
}
})
}
Expand Down

0 comments on commit 60b7d1f

Please sign in to comment.