Skip to content
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

Merged
merged 7 commits into from May 10, 2022
Merged

Conversation

shane-kerr
Copy link
Contributor

@shane-kerr shane-kerr commented Apr 12, 2022

PR for #1361

This code adds more comprehensive parsing and string conversion to the alpn support in the HTTPS/SVCB types.

Before we were just splitting on ,, but this version actually parses as per the draft, which allows alpn with a , in the value. This adds a ton of error cases and a fair bit of code, so I'm hoping the RFC simplifies this to just forbidding , in the alpn completely (am I too late to suggest that?). On the plus side, the examples in the tests now actually return correct values.

I used the existing nextByte() function in the types.go file, since I needed the same functionality. This means that we allow incorrect values like \456, but I figured fixing that was out of scope. Is it worth a separate PR? 馃 Note that the parser for SVCBLocal uses strconv.ParseUint() and should actually bounds check, so we have different strictness/interpretations about escaped characters already.

The String() is also updated to handle outputting non-printable values, and also handles the double-escaping specified in the draft. I decided to output \\044 instead of \, so that naive parsers (like the old one) would at least have a chance to split the output into something like what it should be (since there is no , because we use the numeric version). This shouldn't have any impact on the actual meaning, but I did have to update a couple of tests based on this format.

I added a few more tests to sanity check that everything is working as it is supposed to.

I noticed that the "dohpath" support was merged in while I was working on this. This introduces another place where String() will possibly produce incorrect output. 馃槥 I'll have a look tomorrow....

@shane-kerr shane-kerr requested review from miekg and tmthrgd as code owners Apr 12, 2022
@miekg
Copy link
Owner

miekg commented Apr 13, 2022

I thought it as already an RFC, but still an I-D https://datatracker.ietf.org/doc/html/draft-ietf-dnsop-svcb-https-08 then you should be able to get some changes through, esp. with I tried to code this and it was horrible.

svcb.go Outdated
default:
str.WriteByte(e)
}
} else {
Copy link
Owner

@miekg miekg Apr 13, 2022

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 + a continue and the current if-body can be pulled to the left

Copy link
Contributor Author

@shane-kerr shane-kerr Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, done.

svcb.go Outdated
}
esc[i] = str.String()
}
return strings.Join(esc, ",")
Copy link
Owner

@miekg miekg Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just build the string in str that's already a Builder, so we can just add the new string there. + of course some fiddling for the last element

Copy link
Contributor Author

@shane-kerr shane-kerr Apr 14, 2022

Choose a reason for hiding this comment

The 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 SVCBLocal and didn't really think it through. 馃槀 Done!

svcb.go Show resolved Hide resolved
svcb_test.go Outdated
t.Error("failed to parse RR: ", err)
continue
}
if !reflect.DeepEqual(v, rr.(*SVCB).Value[0].(*SVCBAlpn).Alpn) {
Copy link
Owner

@miekg miekg Apr 13, 2022

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

@shane-kerr shane-kerr Apr 14, 2022

Choose a reason for hiding this comment

The 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 reflect would be fine.

@shane-kerr
Copy link
Contributor Author

shane-kerr commented Apr 14, 2022

I think that all of the feedback has been addressed.

@shane-kerr
Copy link
Contributor Author

shane-kerr commented May 2, 2022

As I mentioned on #1366, I think this PR is as reasonable and should be merged. I checked on the IETF dnsop list and apparently this extra escaping is just how it is, because parsing pain is apparently no reason to change a standard. 馃し

Copy link
Owner

@miekg miekg left a comment

2 nits, lgtm

svcb.go Outdated Show resolved Hide resolved
svcb.go Show resolved Hide resolved
miekg
miekg approved these changes May 10, 2022
@miekg miekg merged commit feda877 into miekg:master May 10, 2022
4 checks passed
aanm pushed a commit to cilium/dns that referenced this issue Jul 29, 2022
* Modify the SVCBAlpn to properly parse/print

* Remove debug

* Change SVCB test from reflect to loop

* Refactor SVCB code to reduce indentation

* When stringifying SVCBAlpn, use strings.Builder for whole process

* Update comment in svcb.go

Co-authored-by: Miek Gieben <miek@miek.nl>

* Describe why we use a specific size for the alpn buffer

Co-authored-by: Miek Gieben <miek@miek.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants