New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Properly parse alpn values in SVCB #1363
Changes from 2 commits
4d924b7
95163f0
5427f80
055c371
7be34d1
8e215ac
5bbb2df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -334,13 +334,56 @@ func (s *SVCBMandatory) copy() SVCBKeyValue { | |
// h.Hdr = dns.RR_Header{Name: ".", Rrtype: dns.TypeHTTPS, Class: dns.ClassINET} | ||
// e := new(dns.SVCBAlpn) | ||
// e.Alpn = []string{"h2", "http/1.1"} | ||
// h.Value = append(o.Value, e) | ||
// h.Value = append(h.Value, e) | ||
type SVCBAlpn struct { | ||
Alpn []string | ||
} | ||
|
||
func (*SVCBAlpn) Key() SVCBKey { return SVCB_ALPN } | ||
func (s *SVCBAlpn) String() string { return strings.Join(s.Alpn, ",") } | ||
func (*SVCBAlpn) Key() SVCBKey { return SVCB_ALPN } | ||
|
||
func (s *SVCBAlpn) String() string { | ||
// An ALPN value is a comma-separated list of values, each of which can be | ||
// an arbitrary binary value. In order to allow parsing, the comma and | ||
// backslash characters are themselves excaped. | ||
// | ||
// However, this escaping is done in addition to the normal escaping which | ||
// happens in zone files, meaning that these values must be | ||
// double-escaped. This looks terrible, so if you see a never-ending | ||
// sequence of backslash in a zone file this may be why. | ||
// | ||
// https://datatracker.ietf.org/doc/html/draft-ietf-dnsop-svcb-https-08#appendix-A.1 | ||
esc := make([]string, len(s.Alpn)) | ||
for i, alpn := range s.Alpn { | ||
var str strings.Builder | ||
str.Grow(4 * len(alpn)) | ||
for j := 0; j < len(alpn); j++ { | ||
e := alpn[j] | ||
if ' ' <= e && e <= '~' { | ||
switch e { | ||
// We escape a few characters which may confuse humans or | ||
// parsers. | ||
case '"', ';', ' ': | ||
str.WriteByte('\\') | ||
str.WriteByte(e) | ||
// The comma and backslash characters themselves must be | ||
// doubly-escaped. We use `\\` for the first backslash and | ||
// the escaped numeric value for the other value. We especially | ||
// don't want a comma in the output. | ||
case ',': | ||
str.WriteString(`\\\044`) | ||
case '\\': | ||
str.WriteString(`\\\092`) | ||
default: | ||
str.WriteByte(e) | ||
} | ||
} else { | ||
str.WriteString(escapeByte(e)) | ||
} | ||
} | ||
esc[i] = str.String() | ||
} | ||
return strings.Join(esc, ",") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we just build the string in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is straightforward. I should have done that to begin with, but I started with the code for |
||
} | ||
|
||
func (s *SVCBAlpn) pack() ([]byte, error) { | ||
// Liberally estimate the size of an alpn as 10 octets | ||
|
@@ -375,7 +418,47 @@ func (s *SVCBAlpn) unpack(b []byte) error { | |
} | ||
|
||
func (s *SVCBAlpn) parse(b string) error { | ||
s.Alpn = strings.Split(b, ",") | ||
if len(b) == 0 { | ||
s.Alpn = []string{} | ||
return nil | ||
} | ||
|
||
alpn := []string{} | ||
a := []byte{} | ||
for p := 0; p < len(b); { | ||
c, q := nextByte(b, p) | ||
if q == 0 { | ||
return errors.New("dns: svcbalpn: unterminated escape") | ||
} | ||
p += q | ||
// If we find a comma, we have finished reading an alpn. | ||
if c == ',' { | ||
if len(a) == 0 { | ||
return errors.New("dns: svcbalpn: empty protocol identifier") | ||
} | ||
alpn = append(alpn, string(a)) | ||
miekg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
a = []byte{} | ||
continue | ||
} | ||
// If it's a backslash, we need to handle a comma-separated list. | ||
if c == '\\' { | ||
dc, dq := nextByte(b, p) | ||
if dq == 0 { | ||
return errors.New("dns: svcbalpn: unterminated escape decoding comma-separated list") | ||
} | ||
if dc != '\\' && dc != ',' { | ||
return errors.New("dns: svcbalpn: bad escaped character decoding comma-separated list") | ||
} | ||
p += dq | ||
c = dc | ||
} | ||
a = append(a, c) | ||
} | ||
// Add the final alpn. | ||
if len(a) == 0 { | ||
return errors.New("dns: svcbalpn: last protocol identifier empty") | ||
} | ||
s.Alpn = append(alpn, string(a)) | ||
return nil | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package dns | ||
|
||
import ( | ||
"reflect" | ||
"testing" | ||
) | ||
|
||
|
@@ -95,6 +96,42 @@ func TestDecodeBadSVCB(t *testing.T) { | |
} | ||
} | ||
|
||
func TestPresentationSVCBAlpn(t *testing.T) { | ||
tests := map[string]string{ | ||
"h2": "h2", | ||
"http": "http", | ||
"\xfa": `\250`, | ||
"some\"other,chars": `some\"other\\\044chars`, | ||
} | ||
for input, want := range tests { | ||
e := new(SVCBAlpn) | ||
e.Alpn = []string{input} | ||
if e.String() != want { | ||
t.Errorf("improper conversion with String(), wanted %v got %v", want, e.String()) | ||
} | ||
} | ||
} | ||
|
||
func TestSVCBAlpn(t *testing.T) { | ||
tests := map[string][]string{ | ||
`. 1 IN SVCB 10 one.test. alpn=h2`: {"h2"}, | ||
`. 2 IN SVCB 20 two.test. alpn=h2,h3-19`: {"h2", "h3-19"}, | ||
`. 3 IN SVCB 30 three.test. alpn="f\\\\oo\\,bar,h2"`: {`f\oo,bar`, "h2"}, | ||
`. 4 IN SVCB 40 four.test. alpn="part1,part2,part3\\,part4\\\\"`: {"part1", "part2", `part3,part4\`}, | ||
`. 5 IN SVCB 50 five.test. alpn=part1\,\p\a\r\t2\044part3\092,part4\092\\`: {"part1", "part2", `part3,part4\`}, | ||
} | ||
for s, v := range tests { | ||
rr, err := NewRR(s) | ||
if err != nil { | ||
t.Error("failed to parse RR: ", err) | ||
continue | ||
} | ||
if !reflect.DeepEqual(v, rr.(*SVCB).Value[0].(*SVCBAlpn).Alpn) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just range over the slices, no real need to pull in reflect for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I've refactored this. I figured that since this was test code |
||
t.Errorf("parsing alpn failed, wanted %v got %v", v, rr.(*SVCB).Value[0].(*SVCBAlpn).Alpn) | ||
} | ||
} | ||
} | ||
|
||
func TestCompareSVCB(t *testing.T) { | ||
val1 := []SVCBKeyValue{ | ||
&SVCBPort{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverse this
if
, so the else part is just done + acontinue
and the current if-body can be pulled to the leftThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, done.